[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 13:31, Ian Campbell wrote:
@@ -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.
I've changed it to disable_xl_vif_scripts, which I think is even more
clear.
@@ -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.
Ok, I will add and extra parameter to libxl__initiate_device_remove that
turns on or off the destruction of the front xenstore entries. We will
first call it without removing the fronted, and if that fails we will
call libxl__initiate_device_remove from the callback this time forcing
the removal of the frontend.
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?
Nope, tap doesn't have a backend xenstore entry, only vifs have, so this
is kind of a hack because I was marking a vifs path as tap...
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.
Does this paths sound ok:
/libxl/devices/<domid>/nic/<devid>/udev_disable
/libxl/devices/<domid>/nic/<devid>/type
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
|
