[PATCH RFC 0/5] tools/FreeBSD: enable disk hotplug scripts

[PATCH RFC 0/5] tools/FreeBSD: enable disk hotplug scripts

[Roger Pau Monne]

This series enables the execution of disk hotplug scripts on FreeBSD. In 
order for this to work a patch for the FreeBSD kernel is also needed, which 
can be found at:

https://people.freebsd.org/~royger/freebsd-hotplug/

This series introduces a new xenstore blkback node, called "path" that's 
used by FreeBSD hotplug scripts in order to write the path to the physical 
device or raw image backing the vbd. The rationale behind the addition of 
this node can be found at patches 2 and 3 of this series.

Thanks, Roger.

[PATCH RFC 1/5] libxl: only write physical-device on Linux and NetBSD

[Roger Pau Monne]

physical-device is only used by Linux and NetBSD blkback, there's no need to
write it when the host is using a different OS.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a6eb2df..8c47cff 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2472,10 +2472,12 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                  */
                 if (!disk->script &&
                     disk->backend_domid == LIBXL_TOOLSTACK_DOMID) {
+#if defined(__linux__) || defined(__NetBSD__)
                     int major, minor;
                     if (!libxl__device_physdisk_major_minor(dev, &major, &minor))
                         flexarray_append_pair(back, "physical-device",
                                               libxl__sprintf(gc, "%x:%x", major, minor));
+#endif /* __linux__ || __NetBSD__ */
                 }
 
                 assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH RFC 2/5] blkif: document new blkback path xenstore node

[Roger Pau Monne]

FreeBSD blkback uses the path xenstore node in order to fetch the path to
the underlying backing storage (either a block device or raw image). This
node is set by the hotplug scripts.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
---
 xen/include/public/io/blkif.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
index 6baf7fb..b1e2245 100644
--- a/xen/include/public/io/blkif.h
+++ b/xen/include/public/io/blkif.h
@@ -89,8 +89,15 @@
  *      Values:         string
  *
  *      A free formatted string providing sufficient information for the
- *      backend driver to open the backing device.  (e.g. the path to the
- *      file or block device representing the backing store.)
+ *      hotplug script to attach the device and provide a suitable
+ *      handler (ie: a block device) for blkback to use.
+ *
+ * path
+ *      Values:         string
+ *      Notes: 11
+ *
+ *      A free formatted string that contains the path to the backing device
+ *      for this blkback instance.
  *
  * type
  *      Values:         "file", "phy", "tap"
@@ -385,6 +392,7 @@
  *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
  *(10) The discard-secure property may be present and will be set to 1 if the
  *     backing device supports secure discard.
+ *(11) This node is only present on FreeBSD.
  */
 
 /*
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH RFC 2/5] blkif: document new blkback path xenstore node

[Wei Liu]

On Fri, May 22, 2015 at 11:18:55AM +0200, Roger Pau Monne wrote:
> FreeBSD blkback uses the path xenstore node in order to fetch the path to
> the underlying backing storage (either a block device or raw image). This
> node is set by the hotplug scripts.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
> ---
>  xen/include/public/io/blkif.h | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> index 6baf7fb..b1e2245 100644
> --- a/xen/include/public/io/blkif.h
> +++ b/xen/include/public/io/blkif.h
> @@ -89,8 +89,15 @@
>   *      Values:         string
>   *
>   *      A free formatted string providing sufficient information for the
> - *      backend driver to open the backing device.  (e.g. the path to the
> - *      file or block device representing the backing store.)
> + *      hotplug script to attach the device and provide a suitable
> + *      handler (ie: a block device) for blkback to use.
> + *
> + * path
> + *      Values:         string
> + *      Notes: 11
> + *
> + *      A free formatted string that contains the path to the backing device
> + *      for this blkback instance.

Why can't FreeBSD use the same "physical-device" node as Linux and
NetBSD do? AIUI from reading this doc "physical-device" node already
covers both "file" and "block device", i.e. it's a superset of the
proposed "path" node here.

Wei.

>   *
>   * type
>   *      Values:         "file", "phy", "tap"
> @@ -385,6 +392,7 @@
>   *     than RING_SIZE * BLKIF_MAX_SEGMENTS_PER_REQUEST.
>   *(10) The discard-secure property may be present and will be set to 1 if the
>   *     backing device supports secure discard.
> + *(11) This node is only present on FreeBSD.
>   */
>  
>  /*
> -- 
> 1.9.5 (Apple Git-50.3)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH RFC 2/5] blkif: document new blkback path xenstore node

[Roger Pau Monné]

El 01/06/15 a les 15.24, Wei Liu ha escrit:
> On Fri, May 22, 2015 at 11:18:55AM +0200, Roger Pau Monne wrote:
>> FreeBSD blkback uses the path xenstore node in order to fetch the path to
>> the underlying backing storage (either a block device or raw image). This
>> node is set by the hotplug scripts.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Tim Deegan <tim@xen.org>
>> ---
>>  xen/include/public/io/blkif.h | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
>> index 6baf7fb..b1e2245 100644
>> --- a/xen/include/public/io/blkif.h
>> +++ b/xen/include/public/io/blkif.h
>> @@ -89,8 +89,15 @@
>>   *      Values:         string
>>   *
>>   *      A free formatted string providing sufficient information for the
>> - *      backend driver to open the backing device.  (e.g. the path to the
>> - *      file or block device representing the backing store.)
>> + *      hotplug script to attach the device and provide a suitable
>> + *      handler (ie: a block device) for blkback to use.
>> + *
>> + * path
>> + *      Values:         string
>> + *      Notes: 11
>> + *
>> + *      A free formatted string that contains the path to the backing device
>> + *      for this blkback instance.
> 
> Why can't FreeBSD use the same "physical-device" node as Linux and
> NetBSD do? AIUI from reading this doc "physical-device" node already
> covers both "file" and "block device", i.e. it's a superset of the
> proposed "path" node here.

Not sure why, but "physical-device" is not documented at all in blkif.h.

I don't mind using the "physical-device" node, it's just that both Linux
and NetBSD use it to store the block device major and minor numbers, and
FreeBSD instead needs to store an absolute path (to either a block or
regular file). If people think it's fine to have different formats for
"physical-device" depending on the underlying OS I don't mind using it
on FreeBSD also.

Roger.

Re: [PATCH RFC 2/5] blkif: document new blkback path xenstore node

[Wei Liu]

On Tue, Jun 02, 2015 at 05:22:04PM +0200, Roger Pau Monné wrote:
> El 01/06/15 a les 15.24, Wei Liu ha escrit:
> > On Fri, May 22, 2015 at 11:18:55AM +0200, Roger Pau Monne wrote:
> >> FreeBSD blkback uses the path xenstore node in order to fetch the path to
> >> the underlying backing storage (either a block device or raw image). This
> >> node is set by the hotplug scripts.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Cc: Ian Campbell <ian.campbell@citrix.com>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Tim Deegan <tim@xen.org>
> >> ---
> >>  xen/include/public/io/blkif.h | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> >> index 6baf7fb..b1e2245 100644
> >> --- a/xen/include/public/io/blkif.h
> >> +++ b/xen/include/public/io/blkif.h
> >> @@ -89,8 +89,15 @@
> >>   *      Values:         string
> >>   *
> >>   *      A free formatted string providing sufficient information for the
> >> - *      backend driver to open the backing device.  (e.g. the path to the
> >> - *      file or block device representing the backing store.)
> >> + *      hotplug script to attach the device and provide a suitable
> >> + *      handler (ie: a block device) for blkback to use.
> >> + *
> >> + * path
> >> + *      Values:         string
> >> + *      Notes: 11
> >> + *
> >> + *      A free formatted string that contains the path to the backing device
> >> + *      for this blkback instance.
> > 
> > Why can't FreeBSD use the same "physical-device" node as Linux and
> > NetBSD do? AIUI from reading this doc "physical-device" node already
> > covers both "file" and "block device", i.e. it's a superset of the
> > proposed "path" node here.
> 
> Not sure why, but "physical-device" is not documented at all in blkif.h.
> 

Er...

> I don't mind using the "physical-device" node, it's just that both Linux
> and NetBSD use it to store the block device major and minor numbers, and
> FreeBSD instead needs to store an absolute path (to either a block or
> regular file). If people think it's fine to have different formats for
> "physical-device" depending on the underlying OS I don't mind using it
> on FreeBSD also.
> 

I think you can just document it and claim it your own pet node. :-)

I don't have much say in this field. I'm fine with anything that's
clearly documented.

Wei.

> Roger.

Re: [PATCH RFC 2/5] blkif: document new blkback path xenstore node

[Ian Campbell]

On Mon, 2015-06-01 at 14:24 +0100, Wei Liu wrote:
> On Fri, May 22, 2015 at 11:18:55AM +0200, Roger Pau Monne wrote:
> > FreeBSD blkback uses the path xenstore node in order to fetch the path to
> > the underlying backing storage (either a block device or raw image). This
> > node is set by the hotplug scripts.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Tim Deegan <tim@xen.org>
> > ---
> >  xen/include/public/io/blkif.h | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/include/public/io/blkif.h b/xen/include/public/io/blkif.h
> > index 6baf7fb..b1e2245 100644
> > --- a/xen/include/public/io/blkif.h
> > +++ b/xen/include/public/io/blkif.h
> > @@ -89,8 +89,15 @@
> >   *      Values:         string
> >   *
> >   *      A free formatted string providing sufficient information for the
> > - *      backend driver to open the backing device.  (e.g. the path to the
> > - *      file or block device representing the backing store.)
> > + *      hotplug script to attach the device and provide a suitable
> > + *      handler (ie: a block device) for blkback to use.
> > + *
> > + * path
> > + *      Values:         string
> > + *      Notes: 11
> > + *
> > + *      A free formatted string that contains the path to the backing device
> > + *      for this blkback instance.
> 
> Why can't FreeBSD use the same "physical-device" node as Linux and
> NetBSD do? AIUI from reading this doc "physical-device" node already
> covers both "file" and "block device", i.e. it's a superset of the
> proposed "path" node here.

I was about to ask the same thing in response to the first patch.



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

[PATCH RFC 3/5] libxl/FreeBSD: write blkback "path" node

[Roger Pau Monne]

FreeBSD blkback doesn't use the physical-device xenstore node because it
can handle both block devices and raw files directly. Instead introduce a
new xenstore blkback node that is used by hotplug scripts to write the path
to the block device or raw image.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 8c47cff..a5a8999 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2477,7 +2477,13 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                     if (!libxl__device_physdisk_major_minor(dev, &major, &minor))
                         flexarray_append_pair(back, "physical-device",
                                               libxl__sprintf(gc, "%x:%x", major, minor));
-#endif /* __linux__ || __NetBSD__ */
+#elif defined(__FreeBSD__)
+                    /*
+                     * FreeBSD blkback supports raw image files, so we
+                     * cannot reuse the physical-device node.
+                     */
+                    flexarray_append_pair(back, "path", dev);
+#endif
                 }
 
                 assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH RFC 4/5] hotplug/FreeBSD: add block hotplug script

[Roger Pau Monne]

This is the default hotplug script for block devices. Its only job is to
copy the "params" blkback xenstore node to "path".

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/FreeBSD/Makefile |  2 +-
 tools/hotplug/FreeBSD/block    | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 tools/hotplug/FreeBSD/block

diff --git a/tools/hotplug/FreeBSD/Makefile b/tools/hotplug/FreeBSD/Makefile
index 8dfc90a..52a95c8 100644
--- a/tools/hotplug/FreeBSD/Makefile
+++ b/tools/hotplug/FreeBSD/Makefile
@@ -2,7 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 # Xen script dir and scripts to go there.
-XEN_SCRIPTS = vif-bridge
+XEN_SCRIPTS = vif-bridge block
 
 XEN_SCRIPT_DATA =
 
diff --git a/tools/hotplug/FreeBSD/block b/tools/hotplug/FreeBSD/block
new file mode 100644
index 0000000..782e5c6
--- /dev/null
+++ b/tools/hotplug/FreeBSD/block
@@ -0,0 +1,25 @@
+#!/bin/sh -e
+#
+# FreeBSD hotplug script for attaching disks
+#
+# Parameters:
+#	$1: xenstore backend path of the vbd
+#	$2: action, either "add" or "remove"
+#
+# Environment variables:
+#	None
+#
+
+path=$1
+action=$2
+params=$(xenstore-read "$path/params")
+
+case $action in
+add)
+	xenstore-write $path/path $params
+	exit 0
+	;;
+*)
+	exit 0
+	;;
+esac
-- 
1.9.5 (Apple Git-50.3)


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

[PATCH RFC 5/5] libxl/FreeBSD: add support for disk hotplug scripts

[Roger Pau Monne]

Allow FreeBSD to execute hotplug scripts when attaching disk devices.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_freebsd.c | 114 ++++++++++++++++++++++++++++++++------------
 1 file changed, 83 insertions(+), 31 deletions(-)

diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
index 47c3391..5ff3388 100644
--- a/tools/libxl/libxl_freebsd.c
+++ b/tools/libxl/libxl_freebsd.c
@@ -59,14 +59,36 @@ static int libxl__hotplug_env_nic(libxl__gc *gc, libxl__device *dev, char ***env
     return 0;
 }
 
-static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev, char ***args,
-                              libxl__device_action action)
+static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
+                              char ***args, char ***env,
+                              libxl__device_action action,
+                              int num_exec)
 {
+    libxl_nic_type nictype;
     char *be_path = libxl__device_backend_path(gc, dev);
     char *script;
-    int nr = 0, rc = 0, arraysize = 4;
+    int nr = 0, rc;
 
-    assert(dev->backend_kind == LIBXL__DEVICE_KIND_VIF);
+    rc = libxl__nic_type(gc, dev, &nictype);
+    if (rc) {
+        LOG(ERROR, "error when fetching nic type");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    /*
+     * For PV domains only one pass is needed (because there's no emulated
+     * interface). For HVM domains two passes are needed in order to add
+     * both the PV and the tap interfaces to the bridge.
+     */
+    if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
+        rc = 0;
+        goto out;
+    }
+
+    rc = libxl__hotplug_env_nic(gc, dev, env, num_exec);
+    if (rc)
+        goto out;
 
     script = libxl__xs_read(gc, XBT_NULL,
                             GCSPRINTF("%s/%s", be_path, "script"));
@@ -76,53 +98,83 @@ static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev, char ***args,
         goto out;
     }
 
+    const int arraysize = 4;
     GCNEW_ARRAY(*args, arraysize);
     (*args)[nr++] = script;
     (*args)[nr++] = be_path;
-    (*args)[nr++] = GCSPRINTF("%s", action == LIBXL__DEVICE_ACTION_ADD ?
-                                    "add" : "remove");
+    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
     (*args)[nr++] = NULL;
     assert(nr == arraysize);
 
+    rc = 1;
+
 out:
     return rc;
 }
 
+static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
+                               char ***args, char ***env,
+                               libxl__device_action action)
+{
+    char *be_path = libxl__device_backend_path(gc, dev);
+    char *script;
+    int nr = 0, rc;
+
+    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;
+    }
+
+    const int arraysize = 4;
+    GCNEW_ARRAY(*args, arraysize);
+    (*args)[nr++] = script;
+    (*args)[nr++] = be_path;
+    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
+    (*args)[nr++] = NULL;
+    assert(nr == arraysize);
+
+    rc = 1;
+
+error:
+    return rc;
+}
+
 int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
                                    char ***args, char ***env,
                                    libxl__device_action action,
                                    int num_exec)
 {
-    libxl_nic_type nictype;
     int rc;
 
-    if (dev->backend_kind != LIBXL__DEVICE_KIND_VIF || num_exec == 2)
-        return 0;
-
-    rc = libxl__nic_type(gc, dev, &nictype);
-    if (rc) {
-        LOG(ERROR, "error when fetching nic type");
-        rc = ERROR_FAIL;
-        goto out;
-    }
-
-    /*
-     * For PV domains only one pass is needed (because there's no emulated
-     * interface). For HVM domains two passes are needed in order to add
-     * both the PV and the tap interfaces to the bridge.
-     */
-    if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
+    switch (dev->backend_kind) {
+    case LIBXL__DEVICE_KIND_VBD:
+        if (num_exec != 0) {
+            rc = 0;
+            goto out;
+        }
+        rc = libxl__hotplug_disk(gc, dev, args, env, action);
+        break;
+    case LIBXL__DEVICE_KIND_VIF:
+        /*
+         * If domain has a stubdom we don't have to execute hotplug scripts
+         * for emulated interfaces
+         */
+        if ((num_exec > 1) ||
+            (libxl_get_stubdom_id(CTX, dev->domid) && num_exec)) {
+            rc = 0;
+            goto out;
+        }
+        rc = libxl__hotplug_nic(gc, dev, args, env, action, num_exec);
+        break;
+    default:
+        /* No need to execute any hotplug scripts */
         rc = 0;
-        goto out;
+        break;
     }
 
-    rc = libxl__hotplug_env_nic(gc, dev, env, num_exec);
-    if (rc)
-        goto out;
-
-    rc = libxl__hotplug_nic(gc, dev, args, action);
-    if (!rc) rc = 1;
-
 out:
     return rc;
 }
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH RFC 5/5] libxl/FreeBSD: add support for disk hotplug scripts

[Ian Campbell]

On Fri, 2015-05-22 at 11:18 +0200, Roger Pau Monne wrote:
> Allow FreeBSD to execute hotplug scripts when attaching disk devices.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

This looks rather similar to the code for Linux, is there any chance
they could be combined into one or more common hotplugger running
things?

> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_freebsd.c | 114 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 83 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/libxl/libxl_freebsd.c b/tools/libxl/libxl_freebsd.c
> index 47c3391..5ff3388 100644
> --- a/tools/libxl/libxl_freebsd.c
> +++ b/tools/libxl/libxl_freebsd.c
> @@ -59,14 +59,36 @@ static int libxl__hotplug_env_nic(libxl__gc *gc, libxl__device *dev, char ***env
>      return 0;
>  }
>  
> -static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev, char ***args,
> -                              libxl__device_action action)
> +static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev,
> +                              char ***args, char ***env,
> +                              libxl__device_action action,
> +                              int num_exec)
>  {
> +    libxl_nic_type nictype;
>      char *be_path = libxl__device_backend_path(gc, dev);
>      char *script;
> -    int nr = 0, rc = 0, arraysize = 4;
> +    int nr = 0, rc;
>  
> -    assert(dev->backend_kind == LIBXL__DEVICE_KIND_VIF);
> +    rc = libxl__nic_type(gc, dev, &nictype);
> +    if (rc) {
> +        LOG(ERROR, "error when fetching nic type");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    /*
> +     * For PV domains only one pass is needed (because there's no emulated
> +     * interface). For HVM domains two passes are needed in order to add
> +     * both the PV and the tap interfaces to the bridge.
> +     */
> +    if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
> +        rc = 0;
> +        goto out;
> +    }
> +
> +    rc = libxl__hotplug_env_nic(gc, dev, env, num_exec);
> +    if (rc)
> +        goto out;
>  
>      script = libxl__xs_read(gc, XBT_NULL,
>                              GCSPRINTF("%s/%s", be_path, "script"));
> @@ -76,53 +98,83 @@ static int libxl__hotplug_nic(libxl__gc *gc, libxl__device *dev, char ***args,
>          goto out;
>      }
>  
> +    const int arraysize = 4;
>      GCNEW_ARRAY(*args, arraysize);
>      (*args)[nr++] = script;
>      (*args)[nr++] = be_path;
> -    (*args)[nr++] = GCSPRINTF("%s", action == LIBXL__DEVICE_ACTION_ADD ?
> -                                    "add" : "remove");
> +    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
>      (*args)[nr++] = NULL;
>      assert(nr == arraysize);
>  
> +    rc = 1;
> +
>  out:
>      return rc;
>  }
>  
> +static int libxl__hotplug_disk(libxl__gc *gc, libxl__device *dev,
> +                               char ***args, char ***env,
> +                               libxl__device_action action)
> +{
> +    char *be_path = libxl__device_backend_path(gc, dev);
> +    char *script;
> +    int nr = 0, rc;
> +
> +    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;
> +    }
> +
> +    const int arraysize = 4;
> +    GCNEW_ARRAY(*args, arraysize);
> +    (*args)[nr++] = script;
> +    (*args)[nr++] = be_path;
> +    (*args)[nr++] = (char *) libxl__device_action_to_string(action);
> +    (*args)[nr++] = NULL;
> +    assert(nr == arraysize);
> +
> +    rc = 1;
> +
> +error:
> +    return rc;
> +}
> +
>  int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>                                     char ***args, char ***env,
>                                     libxl__device_action action,
>                                     int num_exec)
>  {
> -    libxl_nic_type nictype;
>      int rc;
>  
> -    if (dev->backend_kind != LIBXL__DEVICE_KIND_VIF || num_exec == 2)
> -        return 0;
> -
> -    rc = libxl__nic_type(gc, dev, &nictype);
> -    if (rc) {
> -        LOG(ERROR, "error when fetching nic type");
> -        rc = ERROR_FAIL;
> -        goto out;
> -    }
> -
> -    /*
> -     * For PV domains only one pass is needed (because there's no emulated
> -     * interface). For HVM domains two passes are needed in order to add
> -     * both the PV and the tap interfaces to the bridge.
> -     */
> -    if (nictype == LIBXL_NIC_TYPE_VIF && num_exec != 0) {
> +    switch (dev->backend_kind) {
> +    case LIBXL__DEVICE_KIND_VBD:
> +        if (num_exec != 0) {
> +            rc = 0;
> +            goto out;
> +        }
> +        rc = libxl__hotplug_disk(gc, dev, args, env, action);
> +        break;
> +    case LIBXL__DEVICE_KIND_VIF:
> +        /*
> +         * If domain has a stubdom we don't have to execute hotplug scripts
> +         * for emulated interfaces
> +         */
> +        if ((num_exec > 1) ||
> +            (libxl_get_stubdom_id(CTX, dev->domid) && num_exec)) {
> +            rc = 0;
> +            goto out;
> +        }
> +        rc = libxl__hotplug_nic(gc, dev, args, env, action, num_exec);
> +        break;
> +    default:
> +        /* No need to execute any hotplug scripts */
>          rc = 0;
> -        goto out;
> +        break;
>      }
>  
> -    rc = libxl__hotplug_env_nic(gc, dev, env, num_exec);
> -    if (rc)
> -        goto out;
> -
> -    rc = libxl__hotplug_nic(gc, dev, args, action);
> -    if (!rc) rc = 1;
> -
>  out:
>      return rc;
>  }



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

Re: [PATCH RFC 5/5] libxl/FreeBSD: add support for disk hotplug scripts

[Roger Pau Monné]

El 17/06/15 a les 15.23, Ian Campbell ha escrit:
> On Fri, 2015-05-22 at 11:18 +0200, Roger Pau Monne wrote:
>> Allow FreeBSD to execute hotplug scripts when attaching disk devices.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This looks rather similar to the code for Linux, is there any chance
> they could be combined into one or more common hotplugger running
> things?

Although much of the code is similar, the syntax of the disk hotplug
script call is different on Linux vs FreeBSD. Linux calls the hotplug
script with a single parameter (add or remove) and passes a bunch of
data in the env. FreeBSD OTOH passes the backend xenstore path as the
first argument and doesn't set any env variables at all.

Both paths could be merged, but I think the resulting code is going to
be a little messy, and unless people is really careful Linux changes
could break FreeBSD or the other way around, and there's a great chance
people are not going to test on both Linux and FreeBSD.

So, IMHO I would prefer to keep them separated for now.

Roger.



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