Mailing List Archive

[PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl
# HG changeset patch
# User Roger Pau Monne <roger.pau@entel.upc.edu>
# 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@entel.upc.edu>

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

ifeq ($(CONFIG_NetBSD),y)
LIBXL_OBJS-y += libxl_phybackend.o
+LIBXL_OBJS-y += hotplug_netbsd.o
else
LIBXL_OBJS-y += libxl_nophybackend.o
+LIBXL_OBJS-y += hotplug_linux.o
endif

LIBXL_LIBS += -lyajl
diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/hotplug_linux.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/hotplug_linux.c Fri Sep 30 14:38:55 2011 +0200
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <roger.pau@entel.upc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_internal.h"
+
+int libxl_disk_hotplug(libxl__gc *gc, libxl__device *dev)
+{
+ return 0;
+}
+
+int libxl_nic_hotplug_connect(libxl__gc *gc, libxl__device *dev)
+{
+ return 0;
+}
diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/hotplug_netbsd.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/hotplug_netbsd.c Fri Sep 30 14:38:55 2011 +0200
@@ -0,0 +1,129 @@
+/*
+ * Copyright (C) 2011
+ * Author Roger Pau Monne <roger.pau@entel.upc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include <sys/stat.h>
+
+#include "libxl_internal.h"
+
+int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *be_path = libxl__device_backend_path(gc, dev);
+ struct stat stab;
+ char *stype, *sstate, *script, *params;
+ char **args;
+ int nr = 0;
+ flexarray_t *f_args;
+
+ script = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", be_path, "script"));
+ if (!script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to read script from %s",
+ be_path);
+ return -1;
+ }
+
+ params = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", be_path, "params"));
+ if (!params)
+ return -1;
+
+ sstate = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", be_path, "state"));
+ if (!sstate) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to read state from %s",
+ be_path);
+ return -1;
+ }
+
+ if (stat(params, &stab) < 0) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to get stat info\n");
+ return -1;
+ }
+ if (S_ISBLK(stab.st_mode))
+ stype = "phy";
+ if (S_ISREG(stab.st_mode))
+ stype = "file";
+ if (stype == NULL) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Not block or regular file");
+ return -1;
+ }
+
+ f_args = flexarray_make(5, 1);
+ if (!f_args)
+ return -1;
+
+ flexarray_set(f_args, nr++, script);
+ flexarray_set(f_args, nr++, be_path);
+ flexarray_set(f_args, nr++, sstate);
+ flexarray_set(f_args, nr++, stype);
+ flexarray_set(f_args, nr++, NULL);
+
+ args = (char **) flexarray_contents(f_args);
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Calling disk hotplug script: %s %s %s %s",
+ args[0], args[1], args[2], args[3]);
+ if (libxl__forkexec(gc, -1, -1, -1, args) < 0) {
+ free(args);
+ return -1;
+ }
+
+ free(args);
+ return 0;
+}
+
+int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev)
+{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
+ char *be_path = libxl__device_backend_path(gc, dev);
+ char *sstate, *script;
+ char **args;
+ int nr = 0;
+ int rc = -1;
+ flexarray_t *f_args;
+
+ script = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", be_path, "script"));
+ if (!script) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to read script from %s",
+ be_path);
+ return -1;
+ }
+
+ sstate = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", be_path, "state"));
+ if (!sstate) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Unable to read state from %s",
+ be_path);
+ return -1;
+ }
+
+ f_args = flexarray_make(4, 1);
+ if (!f_args)
+ return -1;
+
+ flexarray_set(f_args, nr++, script);
+ flexarray_set(f_args, nr++, be_path);
+ flexarray_set(f_args, nr++, sstate);
+ flexarray_set(f_args, nr++, NULL);
+
+ args = (char **) flexarray_contents(f_args);
+
+ LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Calling nic hotplug script: %s %s %s",
+ args[0], args[1], args[2]);
+ if (libxl__forkexec(gc, -1, -1, -1, args) < 0) {
+ goto out;
+ }
+ rc = 0;
+
+out:
+ free(args);
+ return rc;
+}
diff -r 4ad998fddb16 -r 1d81d1c4c851 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Fri Nov 18 11:29:14 2011 +0100
+++ b/tools/libxl/libxl.c Fri Sep 30 14:38:55 2011 +0200
@@ -1018,6 +1018,11 @@ int libxl_device_disk_add(libxl_ctx *ctx
flexarray_append(back, "params");
flexarray_append(back, dev);

+ flexarray_append(back, "script");
+ flexarray_append(back, libxl__sprintf(&gc, "%s/%s",
+ libxl_xen_script_dir_path(),
+ "block"));
+
assert(device.backend_kind == LIBXL__DEVICE_KIND_VBD);
break;
case LIBXL_DISK_BACKEND_TAP:
@@ -1094,6 +1099,15 @@ int libxl_device_disk_add(libxl_ctx *ctx
goto out_free;
}
}
+
+ /* Call hotplug scripts to attach device */
+ if (libxl__device_execute_hotplug(&gc, &device) < 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for disk: %s\n",
+ disk->pdev_path);
+ rc = -1;
+ goto out_free;
+ }
+
rc = 0;

out_free:
@@ -1556,6 +1570,15 @@ int libxl_device_nic_add(libxl_ctx *ctx,
goto out_free;
}
}
+
+ /* Call hotplug scripts to attach device */
+ if (libxl__device_execute_hotplug(&gc, &device) < 0) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to execute hotplug script for nic: %s\n",
+ nic->ifname);
+ rc = -1;
+ goto out_free;
+ }
+
rc = 0;

out_free:
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)
+{
+ 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
+ */
+ 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@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl [ In reply to ]
On Fri, 2011-11-18 at 11:59 +0000, Roger Pau Monne wrote:
> # HG changeset patch
> # User Roger Pau Monne <roger.pau@entel.upc.edu>
> # 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@entel.upc.edu>
>
> 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



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

> +{
> + 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?

> + */
> + 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@lists.xensource.com
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl [ In reply to ]
2011/11/18 Ian Campbell <Ian.Campbell@citrix.com>:
> On Fri, 2011-11-18 at 11:59 +0000, Roger Pau Monne wrote:
>> # HG changeset patch
>> # User Roger Pau Monne <roger.pau@entel.upc.edu>
>> # 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@entel.upc.edu>
>>
>> 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.

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

>> +     */
>> +    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@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl [ In reply to ]
On Mon, 2011-11-21 at 11:42 +0000, Roger Pau Monné wrote:
> 2011/11/18 Ian Campbell <Ian.Campbell@citrix.com>:
> > On Fri, 2011-11-18 at 11:59 +0000, Roger Pau Monne wrote:
> >> # HG changeset patch
> >> # User Roger Pau Monne <roger.pau@entel.upc.edu>
> >> # 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@entel.upc.edu>
> >>
> >> 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@lists.xensource.com
> >> http://lists.xensource.com/xen-devel
> >
> >
> >



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl [ In reply to ]
I was just looking at this again because someone was talking about
hotplug issues on Linux and something new occurred to me.

On Fri, 2011-11-18 at 11:59 +0000, Roger Pau Monne wrote:
> [...]
> +++ b/tools/libxl/hotplug_linux.c Fri Sep 30 14:38:55 2011 +0200
> [...]
> +int libxl_disk_hotplug(libxl__gc *gc, libxl__device *dev)
> [...]
> +int libxl_nic_hotplug_connect(libxl__gc *gc, libxl__device *dev)
> [...]
> +++ b/tools/libxl/hotplug_netbsd.c Fri Sep 30 14:38:55 2011 +0200
> [...]
> +int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev)
> [...]
> +int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev)

Although Linux doesn't do hotplug this way now I think it will/should in
the future and the code will look very much like the netbsd one. So I
think that the code which actually handles calling the script should
probably be a generic function, only the selector on whether to do so
needs to be platform specific.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Re: [PATCH 6 of 9 v2] libxl: execute hotplug scripts directly from libxl [ In reply to ]
2011/11/24 Ian Campbell <Ian.Campbell@citrix.com>:
> I was just looking at this again because someone was talking about
> hotplug issues on Linux and something new occurred to me.
>
> On Fri, 2011-11-18 at 11:59 +0000, Roger Pau Monne wrote:
>> [...]
>> +++ b/tools/libxl/hotplug_linux.c       Fri Sep 30 14:38:55 2011 +0200
>> [...]
>> +int libxl_disk_hotplug(libxl__gc *gc, libxl__device *dev)
>> [...]
>> +int libxl_nic_hotplug_connect(libxl__gc *gc, libxl__device *dev)
>> [...]
>> +++ b/tools/libxl/hotplug_netbsd.c      Fri Sep 30 14:38:55 2011 +0200
>> [...]
>> +int libxl__disk_hotplug(libxl__gc *gc, libxl__device *dev)
>> [...]
>> +int libxl__nic_hotplug(libxl__gc *gc, libxl__device *dev)
>
> Although Linux doesn't do hotplug this way now I think it will/should in
> the future and the code will look very much like the netbsd one. So I
> think that the code which actually handles calling the script should
> probably be a generic function, only the selector on whether to do so
> needs to be platform specific.

Sorry for the delay, I've been ill most of this week. From what I saw,
the problem is that NetBSD and Linux hotplug scripts have different
parameters, that's why I've split the calling functions into two
different files, and made them OS-dependent. I think I said that
before, but I would be good to agree on the calling parameters passed
to hotplug scripts. Currently NetBSD has the following parameters:

* block: <backend path> <status> <type>
where <type> can be "file" or "phy"

* vif-bridge: <backend path> <status>

* vif-ip: <backend path> <status>

Also Linux hotplug scripts need a good clean up, hard tabs and spaces
are all mixed, and makes the code really hard to understand.

Regards, Roger.

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