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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Sep 09, 2014 at 12:30:33PM +0100, Ian Campbell wrote:
> 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.
> 

OK.

> > ---
> >  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.
> 

OK.

> > +    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.
> 

Yes you're right. I will fix this.

Wei.

> Ian.

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