[PATCH] libxl: do not call default block script

[PATCH] libxl: do not call default block script

[Marek Marczykowski]

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

Re: [PATCH] libxl: do not call default block script

[Ian Campbell]

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);

Re: [PATCH] libxl: do not call default block script

[Roger Pau Monné]

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);
> 

Re: [PATCH] libxl: do not call default block script

[Ian Campbell]

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.

Re: [PATCH] libxl: do not call default block script

[Roger Pau Monné]

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.
> 
> 

Re: [PATCH] libxl: do not call default block script

[Ian Campbell]

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.