|
[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 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?
> 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?
>
> 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?
> =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?
> + 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.
> 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?
> + 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.
> + 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.
Actually, now I think of it, that is also true of the udev_disable key?
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?
> + }
> + 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
> + 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
> +
> 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |