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);
Roger Pau Monné
2013-May-10 14:19 UTC
Re: [PATCH] libxl: do not call default block script
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.clibxl_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.
Roger Pau Monné
2013-May-10 14:29 UTC
Re: [PATCH] libxl: do not call default block script
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.