Re: [Xen-devel] [PATCH] libxl: make libxl_cdrom_insert async

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-07-24 at 17:49 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH] libxl: make libxl_cdrom_insert async"):
> > libxl: make libxl_cdrom_insert async.
> > 
> > This functionality is a bit of a mess and several configurations are
> > not properly supported.
> >
> > [explanation]
> 
> Urgh.
> 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c   Tue Jul 24 14:08:09 2012 +0100
> > +++ b/tools/libxl/libxl.c   Tue Jul 24 17:09:47 2012 +0100
> > @@ -2131,17 +2131,50 @@ int libxl_device_disk_getinfo(libxl_ctx 
> >      return 0;
> >  }
> >  
> > -int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> > *disk)
> > +int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> > *disk,
> > +                       const libxl_asyncop_how *ao_how)
> >  {
> ...
> > +    if (!disk->pdev_path) {
> > +        disk->pdev_path = strdup("");
> 
> Don't you mean libxl__strdup ?  And presumably you should free the
> previous pdev_path ?
> 
> (TBH this massaging of the caller's libxl_device_disk is a bit
> unfriendly but I don't think getting rid of that is critical at this
> stage.  We can constify it later, in 4.3.)
> 
> > +        disk->format = LIBXL_DISK_FORMAT_EMPTY;
> >      }
> ...
> > +    flexarray_append_pair(eject, "type",
> > +                          
> > libxl__device_disk_string_of_backend(disk->backend));
> > +    flexarray_append_pair(eject, "params", "");
> > +    rc = libxl__xs_writev_atonce(gc, path,
> > +                        libxl__xs_kvs_of_flexarray(gc, eject, 
> > eject->count));
> > +    if (rc) goto out;
> > +
> > +    if (disk->format != LIBXL_DISK_FORMAT_EMPTY)
> > +    {
> > +        insert = flexarray_make(4, 1);
> > +
> > +        flexarray_append_pair(insert, "type",
> > +                        
> > libxl__device_disk_string_of_backend(disk->backend));
> > +        flexarray_append_pair(insert, "params",
> > +                        libxl__sprintf(gc, "%s:%s",
> 
> GCSPRINTF ?

Good idea.

> > +                            
> > libxl__device_disk_string_of_format(disk->format),
> > +                            disk->pdev_path));
> > +        rc = libxl__xs_writev_atonce(gc, path,
> > +                        libxl__xs_kvs_of_flexarray(gc, insert, 
> > insert->count));
> > +    }
> 
> This doesn't look actually /incorrect/ but I'm a bit suspicious: if
> changing to a new non-empty disk, you first write params := "", in one
> xenstore transaction, and then the non-empty value in a second
> transaction.  Why ?

Mostly because xend did it that way, I agree that it seems pointless
without adding any additional interlock to the protocol.

At one point I was hoping that copying xend closely would lead to
working stubdom support etc, but that turned out to be a pipe dream.

Just writing the appropriate key once did seem to work, at least in the
cases where it works at all, in my experiments so I guess I might as
well change to that here.

> > diff -r 9ff37d4565de -r a242083e323a tools/libxl/libxl_xshelp.c
> > --- a/tools/libxl/libxl_xshelp.c    Tue Jul 24 14:08:09 2012 +0100
> > +++ b/tools/libxl/libxl_xshelp.c    Tue Jul 24 17:09:47 2012 +0100
> > @@ -61,6 +61,35 @@ int libxl__xs_writev(libxl__gc *gc, xs_t
> >      return 0;
> >  }
> >  
> > +int libxl__xs_writev_atonce(libxl__gc *gc,
> > +                            const char *dir, char *kvs[])
> > +{
> ...
> > +out:
> > +
> > +    /* rc < 0: error
> > +     * rc == 0: ok, we are done
> > +     * rc == +1: need to keep waiting
> > +     */
> 
> This comment has crept across from switch_logdirty_xswatch (which is a
> watch callback function) and isn't correct here.

Ooops, I'll nuke it.

Ian.


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