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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

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.

BTW if you find the macros can't meet your need you can either extend
them or not use them.

> >  
> > > +out: 
> > > +    if (lock) libxl__unlock_domain_userdata(lock); 
> > > +    libxl_device_usbctrl_dispose(&usbctrl_saved); 
> > > +    libxl_domain_config_dispose(&d_config); 
> > > +    return rc; 
> > > +} 
> > > + 
> > > +int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid, 
> > > +                              libxl_device_usbctrl *usbctrl) 
> > > +{ 
> > > +    int rc = 0; 
> > > + 
> > > +    rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl); 
> > > +    if (rc < 0) goto out; 
> > > + 
> > > +    if (usbctrl->devid == -1) { 
> > > +        usbctrl->devid = libxl__device_nextid(gc, domid, "vusb"); 
> > > +        if (usbctrl->devid < 0) { 
> > > +            rc = ERROR_FAIL; 
> > > +            goto out; 
> > > +        } 
> > > +    } 
> > > + 
> > > +    if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl) < 0){ 
> > > +        rc = ERROR_FAIL; 
> > > +        goto out; 
> > > +    } 
> > > + 
> > > +out: 
> > > +    return rc; 
> > > +} 
> > > + 
> > > +int libxl_device_usbctrl_add(libxl_ctx *ctx, uint32_t domid, 
> > > +                             libxl_device_usbctrl *usbctrl, 
> > > +                             const libxl_asyncop_how *ao_how) 
> > > +{ 
> > > +    AO_CREATE(ctx, domid, ao_how); 
> > > +    int rc; 
> > > + 
> > > +    rc = libxl__device_usbctrl_add(gc, domid, usbctrl); 
> >  
> > Hmm... Your remove function is async while this one is sync, why?
> 
> Hmm, maybe not proper here, just referred to pci_add implementation.
> Current calling places only use sync mode.
>  

Yeah, I only ask for consistency.

Wei.

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