Re: [Xen-devel] [PATCH] libxl: Auto-assign NIC devids in initiate_domain_create

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-01-09 at 11:33 +0100, Stefan Bader wrote:
> This appeared to be working on a quick test with a caller leaving
> all devids unset when starting an HVM guest and one that sets them
> all. Possible optimazations (maybe nice to have but probably not
> important):
> 1. something more complicated to find gaps in devids
> 2. limit auto-assignment in initiate_domain_create to HVM domains
> 
> -Stefan
> 
> From bafc8f62ee3e3175ec4d978bceba4b5f891a597d Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> Date: Wed, 8 Jan 2014 18:26:59 +0100
> Subject: [PATCH] libxl: Auto-assign NIC devids in initiate_domain_create
> 
> This will change initiate_domain_create to walk through NIC definitions
> and automatically assign devids to those which have not assigned one.
> The devids are needed later in domcreate_launch_dm (for HVM domains
> using emulated NICs). The command string for starting the device-model
> has those ids as part of its arguments.
> Assignment of devids in the hotplug case is handled by libxl_device_nic_add
> but that would be called too late in the startup case.
> I also moved the call to libxl__device_nic_setdefault here as this seems
> to be the only path leading there and avoids doing the loop a third time.
> The two loops are trying to handle a case where the caller sets some devids
> (not sure that should be valid) but leaves some unset.
> 
> Signed-off-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

I think from a release point of view we should take this since it is a
bug fix to the API which at least libvirt has tripped over (although
libvirt has worked around it, others may not have done so).

Ian J: Does that make sense?

> ---
>  tools/libxl/libxl_create.c |   35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e03bb55..543e0c8 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -706,6 +706,7 @@ static void initiate_domain_create(libxl__egc *egc,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      uint32_t domid;
>      int i, ret;
> +    size_t last_devid = -1;
>  
>      /* convenience aliases */
>      libxl_domain_config *const d_config = dcs->guest_config;
> @@ -746,6 +747,29 @@ static void initiate_domain_create(libxl__egc *egc,
>      libxl_device_disk *bootdisk =
>          d_config->num_disks > 0 ? &d_config->disks[0] : NULL;
>  
> +    /*
> +     * The devid has to be set before launching the device model. For the
> +     * hotplug case this is done in libxl_device_nic_add but on domain
> +     * creation this is called too late.
> +     * Make two runs over configured NICs in order to avoid duplicate IDs
> +     * in case the caller partially assigned IDs.
> +     */
> +    for (i = 0; i < d_config->num_nics; i++) {
> +        /* We have to init the nic here, because we still haven't
> +         * called libxl_device_nic_add when domcreate_launch_dm gets called,
> +         * but qemu needs the nic information to be complete.
> +         */
> +        ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid);
> +        if (ret) goto error_out;
> +
> +        if (d_config->nics[i].devid > last_devid)
> +            last_devid = d_config->nics[i].devid;
> +    }
> +    for (i = 0; i < d_config->num_nics; i++) {
> +        if (d_config->nics[i].devid < 0)
> +            d_config->nics[i].devid = ++last_devid;
> +    }
> +
>      if (restore_fd >= 0) {
>          LOG(DEBUG, "restoring, not running bootloader\n");
>          domcreate_bootloader_done(egc, &dcs->bl, 0);
> @@ -1058,17 +1082,6 @@ static void domcreate_launch_dm(libxl__egc *egc, 
> libxl__multidev *multidev,
>          }
>      }
>  
> -
> -
> -    for (i = 0; i < d_config->num_nics; i++) {
> -        /* We have to init the nic here, because we still haven't
> -         * called libxl_device_nic_add at this point, but qemu needs
> -         * the nic information to be complete.
> -         */
> -        ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid);
> -        if (ret)
> -            goto error_out;
> -    }
>      switch (d_config->c_info.type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>      {



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel