Xen devel - May 2013 - [PATCH] libxl: do not call default block script

When user didn''t specified disk->script, libxl writes
physical-device
node itself, making script call redundant - especially the default one.

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl.c       | 8 +++++---
 tools/libxl/libxl_linux.c | 5 ++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 6733e71..58b7a03 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2093,9 +2093,11 @@ static void device_disk_add(libxl__egc *egc, uint32_t
domid,
                 flexarray_append(back, "params");
                 flexarray_append(back, dev);
 
-                script = libxl__abs_path(gc, disk->script?:
"block",
-                                         libxl__xen_script_dir_path());
-                flexarray_append_pair(back, "script", script);
+                if (disk->script) {
+                    script = libxl__abs_path(gc, disk->script,
+                            libxl__xen_script_dir_path());
+                    flexarray_append_pair(back, "script", script);
+                }
 
                 /* If the user did not supply a block script then we
                  * write the physical-device node ourselves.
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index 115332a..923a1d0 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -334,9 +334,8 @@ static int libxl__hotplug_disk(libxl__gc *gc, libxl__device
*dev,
     script = libxl__xs_read(gc, XBT_NULL,
                             GCSPRINTF("%s/%s", be_path,
"script"));
     if (!script) {
-        LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
-        rc = ERROR_FAIL;
-        goto error;
+        LOG(INFO, "no script for %s", be_path);
+        return 0;
     }
 
     *env = get_hotplug_env(gc, script, dev);
-- 
1.8.1.4

On Wed, 2013-05-08 at 06:07 +0100, Marek Marczykowski
wrote:
> When user didn''t specified disk->script, libxl writes
physical-device
> node itself, making script call redundant - especially the default one.
Redundant, but not harmful in any way, right?

IOW this is 4.4 material?
> 
> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl.c       | 8 +++++---
>  tools/libxl/libxl_linux.c | 5 ++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 6733e71..58b7a03 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2093,9 +2093,11 @@ static void device_disk_add(libxl__egc *egc,
uint32_t domid,
>                  flexarray_append(back, "params");
>                  flexarray_append(back, dev);
>  
> -                script = libxl__abs_path(gc, disk->script?:
"block",
> -                                         libxl__xen_script_dir_path());
> -                flexarray_append_pair(back, "script", script);
> +                if (disk->script) {
> +                    script = libxl__abs_path(gc, disk->script,
> +                            libxl__xen_script_dir_path());
> +                    flexarray_append_pair(back, "script",
script);
> +                }
>  
>                  /* If the user did not supply a block script then we
>                   * write the physical-device node ourselves.
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 115332a..923a1d0 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
> @@ -334,9 +334,8 @@ static int libxl__hotplug_disk(libxl__gc *gc,
libxl__device *dev,
>      script = libxl__xs_read(gc, XBT_NULL,
>                              GCSPRINTF("%s/%s", be_path,
"script"));
>      if (!script) {
> -        LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
> -        rc = ERROR_FAIL;
> -        goto error;
> +        LOG(INFO, "no script for %s", be_path);
> +        return 0;
>      }
>  
>      *env = get_hotplug_env(gc, script, dev);

On 08/05/13 07:07, Marek Marczykowski wrote:
> When user didn''t specified disk->script, libxl writes
physical-device
> node itself, making script call redundant - especially the default one.
This change will break NetBSD support, please also attach the necessary
NetBSD change or at least mention it in the commit message, so it can be
fixed.

If I''m right, the block script does other things apart from writing the
physical device major/minor, it also checks that no other domain is
using this device. If you want to make the hotplug script call non
redundant I would recommend to prevent libxl from witting physical
device major/minor instead of removing the execution of the hotplug script.
> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl.c       | 8 +++++---
>  tools/libxl/libxl_linux.c | 5 ++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 6733e71..58b7a03 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2093,9 +2093,11 @@ static void device_disk_add(libxl__egc *egc,
uint32_t domid,
>                  flexarray_append(back, "params");
>                  flexarray_append(back, dev);
>  
> -                script = libxl__abs_path(gc, disk->script?:
"block",
> -                                         libxl__xen_script_dir_path());
> -                flexarray_append_pair(back, "script", script);
> +                if (disk->script) {
> +                    script = libxl__abs_path(gc, disk->script,
> +                            libxl__xen_script_dir_path());
> +                    flexarray_append_pair(back, "script",
script);
> +                }
>  
>                  /* If the user did not supply a block script then we
>                   * write the physical-device node ourselves.
> diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
> index 115332a..923a1d0 100644
> --- a/tools/libxl/libxl_linux.c
> +++ b/tools/libxl/libxl_linux.c
libxl_netbsd.c should also be modified to reflect this change.
> @@ -334,9 +334,8 @@ static int libxl__hotplug_disk(libxl__gc *gc,
libxl__device *dev,
>      script = libxl__xs_read(gc, XBT_NULL,
>                              GCSPRINTF("%s/%s", be_path,
"script"));
>      if (!script) {
> -        LOGEV(ERROR, errno, "unable to read script from %s",
be_path);
> -        rc = ERROR_FAIL;
> -        goto error;
> +        LOG(INFO, "no script for %s", be_path);
> +        return 0;
>      }
>  
>      *env = get_hotplug_env(gc, script, dev);
>

On Fri, 2013-05-10 at 15:19 +0100, Roger Pau Monne
wrote:
> On 08/05/13 07:07, Marek Marczykowski wrote:
> > When user didn''t specified disk->script, libxl writes
physical-device
> > node itself, making script call redundant - especially the default
one.
> 
> This change will break NetBSD support, please also attach the necessary
> NetBSD change or at least mention it in the commit message, so it can be
> fixed.
I hadn''t thought about NetBSD. This strongly suggests that if it is
appropriate at all it is most definitely 4.4 material since it has
non-obvious affects.
> If I''m right, the block script does other things apart from
writing the
> physical device major/minor, it also checks that no other domain is
> using this device.
I nearly said that too but at least in the Linux scripts the presence of
the physical-device node cause the script to exit before that check.
Oops.
> If you want to make the hotplug script call non
> redundant I would recommend to prevent libxl from witting physical
> device major/minor instead of removing the execution of the hotplug script.
I think this is something we can resolve one way or the other with your
hotplug script rework in 4.4, since it presumably adds new requirements
here.

Ian.

On 10/05/13 16:21, Ian Campbell wrote:
> On Fri, 2013-05-10 at 15:19 +0100, Roger Pau Monne wrote:
>> On 08/05/13 07:07, Marek Marczykowski wrote:
>>> When user didn''t specified disk->script, libxl writes
physical-device
>>> node itself, making script call redundant - especially the default
one.
>>
>> This change will break NetBSD support, please also attach the necessary
>> NetBSD change or at least mention it in the commit message, so it can
be
>> fixed.
> 
> I hadn''t thought about NetBSD. This strongly suggests that if it
is
> appropriate at all it is most definitely 4.4 material since it has
> non-obvious affects.
> 
>> If I''m right, the block script does other things apart from
writing the
>> physical device major/minor, it also checks that no other domain is
>> using this device.
> 
> I nearly said that too but at least in the Linux scripts the presence of
> the physical-device node cause the script to exit before that check.
> Oops.
Ooops, I should have looked at the actual script before speaking so
quickly. It looks like this might cause trouble, specially because we no
longer check that the block device is not opened by two guests at the
same time, IMHO the right solution would be to prevent libxl from
writing the physical-device entry at all, and I''m not sure if this
shouldn''t be considered as a bug fix for 4.3.

NetBSD for example prevents a block device from being opened more than
once at the same time, but I don''t think Linux does this.
> 
>> If you want to make the hotplug script call non
>> redundant I would recommend to prevent libxl from witting physical
>> device major/minor instead of removing the execution of the hotplug
script.
> 
> I think this is something we can resolve one way or the other with your
> hotplug script rework in 4.4, since it presumably adds new requirements
> here.
> 
> Ian.
> 
>

On Fri, 2013-05-10 at 15:29 +0100, Roger Pau Monne
wrote:
> On 10/05/13 16:21, Ian Campbell wrote:
> > On Fri, 2013-05-10 at 15:19 +0100, Roger Pau Monne wrote:
> >> On 08/05/13 07:07, Marek Marczykowski wrote:
> >>> When user didn''t specified disk->script, libxl
writes physical-device
> >>> node itself, making script call redundant - especially the
default one.
> >>
> >> This change will break NetBSD support, please also attach the
necessary
> >> NetBSD change or at least mention it in the commit message, so it
can be
> >> fixed.
> > 
> > I hadn''t thought about NetBSD. This strongly suggests that if
it is
> > appropriate at all it is most definitely 4.4 material since it has
> > non-obvious affects.
> > 
> >> If I''m right, the block script does other things apart
from writing the
> >> physical device major/minor, it also checks that no other domain
is
> >> using this device.
> > 
> > I nearly said that too but at least in the Linux scripts the presence
of
> > the physical-device node cause the script to exit before that check.
> > Oops.
> 
> Ooops, I should have looked at the actual script before speaking so
> quickly. It looks like this might cause trouble, specially because we no
> longer check that the block device is not opened by two guests at the
> same time, IMHO the right solution would be to prevent libxl from
> writing the physical-device entry at all, and I''m not sure if this
> shouldn''t be considered as a bug fix for 4.3.
I seem to vaguely recall some trouble in this area when it was
implemented, which makes me wary of changing it at this stage.

I think it broken localhost live migration or something like that. I
imagine your new hotplug script work will end up also fixing that as a
by-product?
> NetBSD for example prevents a block device from being opened more than
> once at the same time, but I don''t think Linux does this.
I think only if you use O_EXCL.

Ian.