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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 5/20/2015 at 05:04 PM, in message
<20150520090407.GW26335@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>
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. 
>  
> 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. 

Got you and Ian. I'll update codes then.

Chunyan

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