Re: [Xen-devel] [PATCH v1 07/10] libxl: make libxl_cd_insert "eject" + "insert"

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-07-10 at 15:32 +0100, Wei Liu wrote:
> A "cdrom insert" is always processed as "eject" + "insert", with JSON
> config updated in between. So that we can know the correct state of
> CDROM later when we try to retrieve domain configuration: if xenstore is
> "empty", then CDROM is "empty"; otherwise use the information presented
> in JSON.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  tools/libxl/libxl.c          |   48 
> ++++++++++++++++++++++++++++++++++++++++--
>  tools/libxl/libxl_internal.h |   34 ++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 58f07d2..69d94b1 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2654,8 +2654,9 @@ int libxl_device_disk_getinfo(libxl_ctx *ctx, uint32_t 
> domid,
>      return 0;
>  }
>  
> -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> *disk,
> -                       const libxl_asyncop_how *ao_how)
> +static int libxl__cdrom_insert(libxl_ctx *ctx, uint32_t domid,
> +                               libxl_device_disk *disk,
> +                               const libxl_asyncop_how *ao_how)
>  {
>      AO_CREATE(ctx, domid, ao_how);
>      int num = 0, i;
> @@ -2766,6 +2767,49 @@ out:
>      return AO_INPROGRESS;
>  }
>  
> +/*
> + * A "cdrom insert" is always processed as "eject" + "insert", with
> + * updating JSON in between. So that we can know the current state of
> + * CDROM later when we try to retrieve domain configuration: if
> + * xenstore is "empty", then CDROM is "empty"; otherwise use the image
> + * in JSON.

I think you could short circuit the case where the user inserts an empty
disk (AKA ejects), right now you will eject twice. (We don't have an
explicit libxl_cdrom_eject so I presume this is how it is intended to be
done).

I think this function also handles HVM emulated IDE CDROM, which are
semantically a bit different from PV CDROMs in that the empty drive is
still an actual thing. Not sure if/how this impacts on your handling of
the JSON though.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index b4f518a..f8f2ba2 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3355,6 +3355,40 @@ static inline void libxl__update_config_vtpm(libxl__gc 
> *gc,
>          libxl_domain_config_dispose(&d_config);                         \
>      } while (0)
>  
> +/* Search device list for device with the same identifier as "dev",
> + * update that device with the content pointed to by "dev".
> + */
> +#define DEVICE_UPDATE_JSON(type, ptr, cnt, domid, dev, compare, rc)     \

You could define this in terms of REMOVE+ADD, which would match the
behaviour of the actual operation. Slightly less efficient but less code
perhaps?

If not then I wonder how much of these three macros could be made
common.

> +    do {                                                                \
> +        int x;                                                          \
> +        libxl_domain_config d_config;                                   \
> +        bool updated = false;                                           \
> +                                                                        \
> +        if (domid == LIBXL_TOOLSTACK_DOMID)                             \
> +            break;                                                      \
> +                                                                        \
> +        libxl_domain_config_init(&d_config);                            \
> +                                                                        \
> +        rc = libxl__get_domain_configuration(gc, domid, &d_config);     \
> +        if (rc)                                                         \
> +            goto update_done;                                           \
> +                                                                        \
> +        for (x = 0; x < d_config.cnt; x++) {                            \
> +            if (compare(&d_config.ptr[x], (dev))) {                     \
> +                libxl_device_##type##_dispose(&d_config.ptr[x]);        \
> +                libxl_device_##type##_copy(CTX, &d_config.ptr[x],       \
> +                                           (dev));                      \
> +                updated = true;                                         \
> +            }                                                           \
> +        }                                                               \
> +                                                                        \
> +        if (updated)                                                    \
> +            rc = libxl__set_domain_configuration(gc, domid, &d_config); \
> +                                                                        \
> +    update_done:                                                        \
> +        libxl_domain_config_dispose(&d_config);                         \
> +    } while (0)
> +
>  #endif
>  
>  /*



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