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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

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
>    global
>
> > 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
> > switch).
> >
> > * 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) {
> > +    case LIBXL_NIC_TYPE_VIF:
> > +        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;
> > +    case LIBXL_NIC_TYPE_IOEMU:
> > +        /*
> > +         * 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) {
> >         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> >                    "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,
> > {
> > AO_GC;
> > 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));
> > +
> > +retry_transaction:
> > +    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;
> > -    }
> >
> > -retry_transaction:
> > -    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;
> >
> > out_ok:
> > +    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.
Yes.

> > +    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));
> > +            }
> Likewise
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.
Yes.

> > +            /*
> > +             * 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) {
> >            free(nic->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;
> > }
> > --
> > 1.7.7.5 (Apple Git-26)
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel