[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 4/5] libxl: call hotplug scripts from libxl for vif

On 18 Apr 2012, at 10:54, Ian Campbell wrote:

On Tue, 2012-04-17 at 16:51 +0100, Roger Pau Monne wrote:
As the previous change already introduces most of needed machinery to call hotplug scripts from libxl, this only adds the necessary bits to call this
scripts for vif interfaces also.

libxl_device_nic_add has been changed to make use of the new event
functionality, and the necessary vif hotplug code has been added. No changes where needed in the teardown part, since it uses exactly the same code
introduced for vbd.

An exception has been introduced for tap devices, since hotplug scripts should be called after the device model has been created, so the hotplug call for those is done in do_domain_create after confirmation of the device model startup. On the other hand, tap devices don't use teardown scripts,
so add another exception for that.

libxl__device_from_nic was moved to libxl_device.c, so it can be used
in libxl_create.c, and libxl__device_from_disk was also moved to mantain
the simmetry.

"maintain" and "symmetry"

These are pure code motion or did the code actually change too?

The code didn't change, I just moved it around.

libxl__initiate_device_remove has been changed a little, to nuke the frontend xenstore path before trying to perform device teardown, this allows for unitialized devices to be closed gracefully, specially vif interfaces added to

uninitialized (or -ised)

HVM machines and not used.

PV nic devices are set to LIBXL_NIC_TYPE_VIF, since the default value is
LIBXL_NIC_TYPE_IOEMU regardless of the guest type.

A new gobal option, called disable_vif_scripts has been added to allow the user


decide if he wants to execute the hotplug scripts from xl or from udev. This has
been documented in the xl.conf man page.

Did you do this for block too?

Nope, only for vif interfaces, that are the only ones that have some kind of limited support for the driver domains thing when called from udev.

Changes since v1:

* Propagated changes from previous patch (disable_udev and libxl_device_nic_add

* Modified udev rules to add the new env variable.

* Added support for named interfaces.

Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
docs/man/xl.conf.pod.5                |    7 ++
tools/hotplug/Linux/xen-backend.rules |    6 +-
tools/libxl/libxl.c                   |   90 +++++++-------------
tools/libxl/libxl.h                   |    3 +-
tools/libxl/libxl_create.c            |   22 +++++-
tools/libxl/libxl_device.c            |   77 +++++++++++++++--
tools/libxl/libxl_dm.c                |    2 +-
tools/libxl/libxl_internal.h          |    6 ++
tools/libxl/libxl_linux.c | 150 ++++++++++++++++++++++++++++++++-
tools/libxl/libxl_types.idl           |    1 +
tools/libxl/xl.c                      |    4 +
tools/libxl/xl.h                      |    1 +
tools/libxl/xl_cmdimpl.c              |   15 +++-
13 files changed, 308 insertions(+), 76 deletions(-)

diff --git a/docs/man/xl.conf.pod.5 b/docs/man/xl.conf.pod.5
index 8bd45ea..cf2c477 100644
--- a/docs/man/xl.conf.pod.5
+++ b/docs/man/xl.conf.pod.5
@@ -55,6 +55,13 @@ default.

Default: C<1>

+=item B<disable_vif_scripts=BOOLEAN>
+If enabled xl will not execute nic hotplug scripts itself, and instead
+relegate this task to udev.
+Default: C<0>

Please can you also add a commented out version
to ./tools/examples/xl.conf, with the value of the default.

This doesn't actually disable the scripts entirely, but rather only from
udev, disable_udev_vif_scripts perhaps?

It actually disables script calling from libxl, so I think it might be best to call it disable_xl_vif_scripts?

=item B<lockfile="PATH">

Sets the path to the lock file used by xl to serialise certain
@@ -1929,14 +1883,30 @@ int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic *nic) libxl__xs_kvs_of_flexarray(gc, back, back->count), libxl__xs_kvs_of_flexarray(gc, front, front->count));

-    /* FIXME: wait for plug */
+    switch(nic->nictype) {
+        if (!nic->disable_vif_script) {
+            rc = libxl__initiate_device_add(egc, ao, &device);
+            if (rc) goto out_free;
+        }

You don't need an ao_complete in the else case?

Sure, good one, I think this kind of mechanism is prone to errorsâ AO_INPROGRESS should call ao_complete if no events have been added.

+        break;
+        /*
+         * Don't execute hotplug scripts for IOEMU interfaces yet,
+         * we need to launch the device model first.
+        */

It'd be useful to add a reference to where these are run, at first I
thought this comment (and the patch to xen-backend.rules) meant they
weren't being run at all.

Done I've added a small note saying where they are called.

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index de598ad..a1f5731 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -607,7 +607,7 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
for (i = 0; i < d_config->num_vifs; i++) {
-        ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i]);
+ ret = libxl_device_nic_add(ctx, domid, &d_config->vifs[i], 0);
    if (ret) {
                   "cannot add nic %d to domain: %d", i, ret);
@@ -685,6 +685,26 @@ static int do_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
                   "device model did not start: %d", ret);
        goto error_out;
+        /*
+ * Execute hotplug scripts for tap devices, this has to be done
+         * after the domain model has been started.
+        */
+        for (i = 0; i < d_config->num_vifs; i++) {
+            if (d_config->vifs[i].nictype == LIBXL_NIC_TYPE_IOEMU &&
+                !d_config->vifs[i].disable_vif_script) {
+                libxl__device device;
+ ret = libxl__device_from_nic(gc, domid, &d_config->vifs[i],
+                                             &device);
+                if (ret < 0) goto error_out;
+                ret = libxl__device_hotplug(gc, &device, CONNECT);

I should have asked this in 3/5 but does this function not need to be
async, since the script might take a while and we need to wait for it to
complete before continuing?

Are you sure we need to wait for it to finish? There wasn't any kind of synchronization in the past when we called the scripts from udev, so I guess we don't have to wait either for them to finish.

+                if (ret < 0) {
+                    LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+ "unable to launch hotplug script for device: "
+                               "%"PRIu32, device.devid);
+                    goto error_out;
+                }
+            }
+        }

for (i = 0; i < d_config->num_pcidevs; i++)
@@ -450,22 +504,29 @@ int libxl__initiate_device_remove(libxl__egc *egc, libxl__ao *ao,
libxl_ctx *ctx = libxl__gc_owner(gc);
-    xs_transaction_t t;
+    xs_transaction_t t = 0;
char *be_path = libxl__device_backend_path(gc, dev);
char *state_path = libxl__sprintf(gc, "%s/state", be_path);
-    char *state = libxl__xs_read(gc, XBT_NULL, state_path);
+    char *state;
int rc = 0;
libxl__ao_device_remove *aorm = 0;

+    /*
+     * Nuke frontend to force backend teardown
+ * It's not pretty, but it's the only way that seems to work for all
+     * types of backends
+     */

I'm still not happy with this. The _remove functions are supposed to be
a graceful + co-operative remove, that means prodding the front and
backend into doing the controlled teardown dance.

This may well take some time and may even fail after some timeout
depending on the device type, guest configuration and co-operation etc,
but that is expected and should be handled.

Forcing things in this way is appropriate for device_destroy only IMHO
since that is the function which is provided for use when the guest is
not co-operating.

Before this patch, we used to just nuke both front and back xenstore directories (because we always called libxl__device_destroy), so I think this is quite and improvement in term of co-operation than what we had before. We only used this kind of negotiation when detaching a block from a live guest.

+ libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
+    t = xs_transaction_start(ctx->xsh);
+    state = libxl__xs_read(gc, t, state_path);
if (!state)
    goto out_ok;
-    if (atoi(state) != 4) {
+    if (atoi(state) == XenbusStateClosed)
    libxl__device_destroy_tapdisk(gc, be_path);
    goto out_ok;
-    }

-    t = xs_transaction_start(ctx->xsh);
xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online", be_path), "0", strlen("0"));
xs_write(ctx->xsh, t, state_path, "5", strlen("5"));
if (!xs_transaction_end(ctx->xsh, t, 0)) {
@@ -476,6 +537,8 @@ retry_transaction:
        goto out_fail;
+ /* mark transaction as ended, to prevent double closing it on out_ok */
+    t = 0;

libxl__device_destroy_tapdisk(gc, be_path);

@@ -497,8 +560,8 @@ retry_transaction:
return rc;

+    if (t) xs_transaction_end(ctx->xsh, t, 0);
rc = libxl__device_hotplug(gc, dev, DISCONNECT);
- libxl__xs_path_cleanup(gc, libxl__device_frontend_path(gc, dev));
libxl__xs_path_cleanup(gc, be_path);
return 0;
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index c5583af..1ef282f 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -27,11 +27,30 @@ int libxl__try_phy_backend(mode_t st_mode)

/* Hotplug scripts helpers */
+static libxl_nic_type get_nic_type(libxl__gc *gc, libxl__device *dev)
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    char *snictype, *be_path;
+    be_path = libxl__device_backend_path(gc, dev);
+    snictype = libxl__xs_read(gc, XBT_NULL,
+ libxl__sprintf(gc, "%s/%s", be_path, "type"));

This really ought to be under some private libxl path relating to the
device rather than under the backend/frontend visible dirs.

Well, this is currently only used by libxl, but the type of the backend used I think should be inside the backend device directory, since other applications might also want to know this.

Actually, now I think of it, that is also true of the udev_disable key?

This is probably true for udev_disable, something like /libxl/devices/<domid>/<devid>/udev_disable? I will have to modify libxl__device_generic_add to do this, because it should be done on the same transaction when the device is added. I'm not sure if we need to add so much overhead for just one xenstore entry, that will go away in the next release probably.

You could also use the handy enum to/from_strings functions to leave one
less magic number in xenstore.


+    if (!snictype) {
+ LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unable to read nictype from %s",
+                                          be_path);
+        return -1;
+    }
+    return atoi(snictype);
@@ -62,6 +81,35 @@ static char **get_hotplug_env(libxl__gc *gc, libxl__device *dev)
              dev->domid, dev->devid));
flexarray_set(f_env, nr++, "XENBUS_BASE_PATH");
flexarray_set(f_env, nr++, "backend");
+    if (dev->backend_kind == LIBXL__DEVICE_KIND_VIF) {
+ ifname = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/%s", + be_path, + "vifname"));
+        switch (get_nic_type(gc, dev)) {
+        case LIBXL_NIC_TYPE_IOEMU:
+            flexarray_set(f_env, nr++, "INTERFACE");
+            if (ifname) {
+ flexarray_set(f_env, nr++, libxl__sprintf(gc, "xentap-%s", + ifname));
+            } else {
+                flexarray_set(f_env, nr++,
+                              libxl__sprintf(gc, "xentap%u.%d",
+                              dev->domid, dev->devid));

This is cut-n-paste of some other code, perhaps create a function to get
the tap name from the vif name and or dom/devid?

Are you going to add it to your patch or I should add it to mine? Something like:

char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev)

+            }
+        case LIBXL_NIC_TYPE_VIF:
+            flexarray_set(f_env, nr++, "vif");
+            if (ifname) {
+                flexarray_set(f_env, nr++, ifname);
+            } else {
+                flexarray_set(f_env, nr++,
+                              libxl__sprintf(gc, "vif%u.%d",
+                              dev->domid, dev->devid));
+            }


char *libxl__device_vif_name(libxl__gc *gc, libxl__device *dev)

Both this functions should return the correct vifname if one is set, or the default one otherwise.

+            break;
+        default:
+            return NULL;
+        }
+    }

flexarray_set(f_env, nr++, NULL);

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 01ff363..41230a6 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -846,6 +846,19 @@ static void parse_config_data(const char *configfile_filename_report,
            nic->script = strdup(default_vifscript);

+            /* Set default nic type for PV guests correctly */
+            if (b_info->type == LIBXL_DOMAIN_TYPE_PV) {
+                nic->nictype = LIBXL_NIC_TYPE_VIF;
+            }

Hrm, really the lib ought to be taking care of that for us...

libxl__device_nic_setdefault has a domid so it should be able to query
the domain type with libxl__domain_type.


+            /*
+             * Disable nic network script calling, to allow the user
+             * to attach the nic backend from a different domain.
+             */
+            if (disable_vif_scripts) {
+                nic->disable_vif_script = 1;
+            }

This is equivalent to :
           nic->disable_vif_script = disable_vif_scripts

Yes, and it's one line less.

       if (default_bridge) {
           nic->bridge = strdup(default_bridge);
@@ -4901,7 +4914,7 @@ int main_networkattach(int argc, char **argv)
        return 1;
-    if (libxl_device_nic_add(ctx, domid, &nic)) {
+    if (libxl_device_nic_add(ctx, domid, &nic, 0)) {
    fprintf(stderr, "libxl_device_nic_add failed.\n");
    return 1;
-- (Apple Git-26)

Xen-devel mailing list

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.