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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-09-04 at 23:43 +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.
> 
> Note that libxl_cd_insert doesn't care about the incarnation of "empty"
> state so I don't state explictly what is in xenstore.
> 
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> change in v3:
> clarify "empty" in commit log

I think you should drop the ""s from around empty, since it keeps making
me thing that it isn't really empty in some sense or otherwise special
that I don't understand.

> ---
>  tools/libxl/libxl.c |   67 
> +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index fa7d5d5..1fdb2d8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2662,8 +2662,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;
> @@ -2774,6 +2775,68 @@ 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.
> + *
> + * Note that we don't actually care about the incarnation of "empty"
> + * state so we don't state explicitly what the content in xenstore
> + * should be.
> + *
> + * In order to maintain lock hierarchy, CTX lock is taken when
> + * entering this function.

If this is because the function takes the userdata lock then you should
say so.

> +    lock = libxl__lock_domain_userdata(gc, domid);
> +    if (!lock) {
> +        rc = ERROR_LOCK_FAIL;
> +        goto out;
> +    }
> +    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> +    if (rc) goto out;
> +    DEVICE_ADD(disk, disks, domid, disk, COMPARE_DISK, &d_config);
> +    rc = libxl__set_domain_configuration(gc, domid, &d_config);
> +    if (rc) goto out;
> +    libxl__unlock_domain_userdata(lock);
> +
> +    rc = libxl__cdrom_insert(ctx, domid, disk, ao_how);
> +
> +out:
> +    libxl_device_disk_dispose(&empty);
> +    libxl_domain_config_dispose(&d_config);

This code doesn't seem to adhere to the protocol laid down in the
previous patch and I suspect it therefore isn't safe against parallel
eject/insert calls.

        INSERTION A             INSERTION B

        takes lock
        updates json
        releases lock

                                takes lock
                                updates json
                                releases lock

                                adds to XS

        adds to XS

Now the json == B and the xenstore == A.

I think you need to push the json update down into libxl__cdrom_insert
and do the say dance you do elsewhere, with an XS transaction +
existence check inside the userdata locked region.

Ian.


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