Re: [PATCH V3 3/6] libxl: add pvusb API

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-05-20 at 10:04 +0100, Wei Liu wrote:
> On Mon, May 18, 2015 at 09:20:43PM -0600, Chun Yan Liu wrote:
> [...]
> > >  
> > > > +    for (;;) { 
> > > > +        rc = libxl__xs_transaction_start(gc, &t); 
> > > > +        if (rc) goto out; 
> > > > + 
> > > > +        rc = libxl__device_exists(gc, t, device); 
> > > > +        if (rc < 0) goto out; 
> > > > +        if (rc == 1) { 
> > > > +            /* already exists in xenstore */ 
> > > > +            LOG(ERROR, "device already exists in xenstore"); 
> > > > +            rc = ERROR_DEVICE_EXISTS; 
> > > > +            goto out; 
> > > > +        } 
> > > > + 
> > > > +        rc = libxl__set_domain_configuration(gc, domid, &d_config); 
> > > > +        if (rc) goto out; 
> > > > + 
> > > > +        libxl__device_generic_add(gc, t, device, 
> > > > +                                  libxl__xs_kvs_of_flexarray(gc, back, 
> > > > +                                                             back->count), 
> > > > +                                  libxl__xs_kvs_of_flexarray(gc, front, 
> > > > +                                                             front->count), 
> > > > +                                  NULL); 
> > > > +        libxl__usbport_add_xenstore(gc, t, domid, usbctrl); 
> > > > +        rc = libxl__xs_transaction_commit(gc, &t); 
> > > > +        if (!rc) break; 
> > > > +        if (rc < 0) goto out; 
> > > > +    } 
> > > > + 
> > >  
> > > You don't have aodev so you cannot check update_json. Maybe you need 
> > > aodev? 
> > >  
> > > That field update_json is set to true when doing hotplug. It's set to 
> > > false during domain creation. 
> > >  
> > > The same comment applies to other add functions as well.
> > 
> > Thanks, I'm updating that. But maybe like pci_add and pci_remove functions,
> > will add a 'starting' flag to indicate hotplug or creation.
> > Looking at DEFINE_DEVICE_ADD and DEFINE_DEVICE_REMOVE, usbctrl_add
> > and usb_add can use DEFINE_DEVICE_ADD; but usbctrl_remove and usb_remove
> > cannot use DEFINE_DEVICE_REMOVE directly, need some extra handling. So,
> > finally turned to not use these macros but refer to pci functions.
> > 
> 
> TBH I prefer to have only one way to deal with devices.  I personally
> prefer the async style that every other devices use. Maybe that's just
> because I mostly worked with those.
> 
> I don't know the full history of libxl_pci.c so I will wait for Ian and
> Ian's comments on which way to go.

libxl_pci.c should not be used as an example of the right way to do
things (to say the least).

> I think one merit of determining whether to use sync or async is that
> whether the operation is long running (slow). Long running one should be
> asyn.  I guess usb passthrough is not slow so we are probably fine in
> this regard.

Device add/remove is expected to be async at least at the public API
level , all the others appear to be and it seems logical to me that they
would be.