Re: [Xen-devel] [PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2011-11-21 at 11:42 +0000, Roger Pau Monnà wrote:
> 2011/11/18 Ian Campbell <Ian.Campbell@xxxxxxxxxx>:
> > On Fri, 2011-11-18 at 11:59 +0000, Roger Pau Monne wrote:
> >> # HG changeset patch
> >> # User Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> >> # Date 1317386335 -7200
> >> # Node ID 1d81d1c4c851c0b07696373304801df56a221e90
> >> # Parent  4ad998fddb16a299cb230ea23ba944d84adbd2bf
> >> libxl: execute hotplug scripts directly from libxl.
> >>
> >> Added the necessary handlers to execute hotplug scripts when necessary
> >> from libxl. Split NetBSD and Linux hotplug calls into two separate
> >> files, because parameters for hotplug scripts are different. Linux
> >> has empty functions, since the calling of hotplug scripts is still
> >> done using udev.
> >>
> >> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxxxxx>
> >>
> >> diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/Makefile
> >> --- a/tools/libxl/Makefile      Fri Nov 18 11:29:14 2011 +0100
> >> +++ b/tools/libxl/Makefile      Fri Sep 30 14:38:55 2011 +0200
> >> @@ -34,8 +34,10 @@ LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpu
> >
> > Should be:
> >
> >>  ifeq ($(CONFIG_NetBSD),y)
> >>  LIBXL_OBJS-y += libxl_phybackend.o
> >> +LIBXL_OBJS-y += hotplug_netbsd.o
> >   elsif ($(CONFIG_Linux),y)
> >>  LIBXL_OBJS-y += libxl_nophybackend.o
> >> +LIBXL_OBJS-y += hotplug_linux.o
> >   else
> >   $(error A message of some sort)
> >>  endif
> 
> Done, I've moved PHY backend selection and hotplug specific functions
> to libxl_netbsd.c and libxl_linux.c, and added an error message in
> case someone tries to use a different OS.
> 
> >> diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_device.c
> >> --- a/tools/libxl/libxl_device.c        Fri Nov 18 11:29:14 2011 +0100
> >> +++ b/tools/libxl/libxl_device.c        Fri Sep 30 14:38:55 2011 +0200
> >> @@ -68,6 +68,24 @@ int libxl__parse_backend_path(libxl__gc
> >>      return libxl__device_kind_from_string(strkind, &dev->backend_kind);
> >>  }
> >>
> >> +int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device *dev)
> >
> > No need for a add/remove type parameter?
> 
> Although you could pass this kind of parameter, the action to perform
> is based on the state of the device in the corresponding xenstore
> entry. State 2 means that the device should be connected, state 6
> means the device should be disconnected.

This seems a little fragile in the face of weird errors happening
asynchronously and could also be racy in the normal case. I think you
should pass in the action to be performed and, if necessary, confirm
that the state is correct. If possible you should just perform the
requested action irrespective of the backend state.

> 
> >> +{
> >> +    int rc = 0;
> >> +
> >> +    switch(dev->kind) {
> >> +    case LIBXL__DEVICE_KIND_VIF:
> >> +        rc = libxl__nic_hotplug(gc, dev);
> >> +        break;
> >> +    case LIBXL__DEVICE_KIND_VBD:
> >> +        rc = libxl__disk_hotplug(gc, dev);
> >> +        break;
> >> +    default:
> >> +        break;
> >> +    }
> >> +
> >> +    return rc;
> >> +}
> >> +
> >>  int libxl__device_generic_add(libxl__gc *gc, libxl__device *device,
> >>                               char **bents, char **fents)
> >>  {
> >> @@ -449,6 +467,7 @@ int libxl__device_remove(libxl__gc *gc,
> >>      if (!state)
> >>          goto out;
> >>      if (atoi(state) != 4) {
> >> +        libxl__device_execute_hotplug(gc, dev);
> >>          libxl__device_destroy_tapdisk(gc, be_path);
> >>          xs_rm(ctx->xsh, XBT_NULL, be_path);
> >>          goto out;
> >> @@ -492,6 +511,12 @@ int libxl__device_destroy(libxl__gc *gc,
> >>      char *be_path = libxl__device_backend_path(gc, dev);
> >>      char *fe_path = libxl__device_frontend_path(gc, dev);
> >>
> >> +    /*
> >> +     * Run hotplug scripts, which will probably not be able to
> >> +     * execute successfully since the device may still be plugged
> >
> > What does this mean? If we don't expect it to work why are we calling
> > them?
> 
> If you call the libxl_domain_destroy function with force == 1, the
> destroy process doesn't wait for devices to be disconnected, but we
> might as well try to execute hotplug scripts, since we don't know the
> actual state of the device. If we are lucky, the device might be
> disconnected (state == 6), and we should be able to execute hotplug
> scripts successfully.

If you pass the action to be performed rather than relying on the device
state then perhaps the scripts have a better chance of working.

However if the script cannot run reliably for a forced shutdown
(presumably you cannot teardown the loop device if it is still in use by
the backend?) then we need to solve that somehow rather than leaking the
resource.

Ian.

> 
> >> +     */
> >> +    libxl__device_execute_hotplug(gc, dev);
> >> +
> >>      xs_rm(ctx->xsh, XBT_NULL, be_path);
> >>      xs_rm(ctx->xsh, XBT_NULL, fe_path);
> >>
> >> diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl_internal.h
> >> --- a/tools/libxl/libxl_internal.h      Fri Nov 18 11:29:14 2011 +0100
> >> +++ b/tools/libxl/libxl_internal.h      Fri Sep 30 14:38:55 2011 +0200
> >> @@ -287,6 +287,34 @@ _hidden int libxl__wait_for_device_state
> >>   */
> >>  _hidden int libxl__try_phy_backend(mode_t st_mode);
> >>
> >> +/* hotplug functions */
> >> +/*
> >> + * libxl__device_execute_hotplug - generic function to execute hotplug 
> >> scripts
> >> + * gc: allocation pool
> >> + * dev: reference to the device that executes the hotplug scripts
> >> + *
> >> + * Returns 0 on success, and < 0 on error.
> >> + */
> >> +_hidden int libxl__device_execute_hotplug(libxl__gc *gc, libxl__device 
> >> *dev);
> >> +
> >> +/*
> >> + * libxl__disk_hotplug - execute hotplug script for a disk type device
> >> + * gc: allocation pool
> >> + * dev: reference to the disk device
> >> + *
> >> + * Returns 0 on success, and < 0 on error.
> >> + */
> >> +_hidden int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev);
> >> +
> >> +/*
> >> + * libxl__nic_hotplug - execute hotplug script for a nic type device
> >> + * gc: allocation pool
> >> + * dev: reference to the nic device
> >> + *
> >> + * Returns 0 on success, and < 0 on error.
> >> + */
> >> +_hidden int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev);
> >> +
> >>  /* from libxl_pci */
> >>
> >>  _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, 
> >> libxl_device_pci *pcidev, int starting);
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@xxxxxxxxxxxxxxxxxxx
> >> http://lists.xensource.com/xen-devel
> >
> >
> >



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel