|
[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
> >> 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.
OK, makes sense.
> >> @@ -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?
Oh, I was confusing it with the xenstore key used by the scripts! I
think your existing name is fine then.
> >
> >> 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?
Not at all..
> 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.
You are right, I don't think xl currently waits.
xend used to (IIRC) wait for the hotplug-status node to be written and
used this to do some basic error handling, waiting doe that node is
roughly equivalent to waiting for the script to finish.
Given that xl today doesn't wait I guess I'd be happy to defer that to
the future.
> >> @@ -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),
Not always, the call to destroy depended on the current state.
In the case where the guest is alive and responsive we have to do the
hot remove in the graceful way. If the guest is alive but not responsive
then the correct approach is to try the graceful method with a timeout
which triggers the big hammer approach.
> 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.
Hang on, can't you infer the type from the backend path, one should
contains vif and the other something else (tap)? Or is this because of
the stupid sharing of the vif dir for both vif and tap from the hotplug
scripts point of view?
It's probably too late in the 4.2 cycle to direct the tap hotplug script
to a different backend dir so I think the best thing to do for now is to
put this key somewhere else so that it doesn't become a guest visible
API (which is what happens with where you have put it). The same place
as udev_disable would work fine.
> > 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.
I think this will be around for more than one release (these
deprecations always take time) so it is worth doing right.
> >
> >> + 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?
I'll let you do it if that's alright.
> Something like:
>
> char *libxl__device_tap_name(libxl__gc *gc, libxl__device *dev)
[...]
> 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.
Yep.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |