On Fri, Dec 6, 2019 at 12:04 PM Frediano Ziglio <fzig...@redhat.com> wrote:
> > > +gboolean > > +spice_usb_device_manager_create_shared_cd_device( > > + SpiceUsbDeviceManager *self, > > + gchar > *filename, > > + GError **err) > > style, see > https://www.spice-space.org/spice-project-coding-style-and-coding-conventions.html > , > here in a lot of declarations > Do you mean argument alignment? Sure, to be fixed. > Also for consistency in the other file "manager" is used, not "self". > There is already no consistency. Some functions have been using 'self', some - 'manager'. I suggest leaving it as is and producing a separate patch dedicated to this style issue so that we don't mix CD stuff with the global style fixes. > It's weird that the create does not return the device so you cannot > easily use spice_usb_device_manager_remove_shared_cd_device to remove it. > Again, Yuri addressed this issue already in his mail. It is a kind of design issue, as this is the way things are done right now. I guess that it was implemented this way due to the asynchronous nature of the connecting process. Anyway, it's a question of the flow management more than of a pure API definition. Of course, I can imagine other types of design, but if necessary at all, such change should be done across the board. > + bdev = spice_usb_device_manager_device_to_bdev(self, device); > > see below > > > +#ifdef USE_USBREDIR > > + SpiceUsbBackendDevice *bdev; > > + gboolean is_cd; > > + > > + bdev = spice_usb_device_manager_device_to_bdev(self, device); > > Note that SpiceUsbBackendDevice is defined as > > typedef struct _SpiceUsbDevice SpiceUsbBackendDevice; > > no need to call this function. > > I agree with Yuri. This is the current practice in all API functions. We can rework it everywhere, or leave it as is. > > + is_cd = (spice_usb_backend_device_get_libdev(bdev) == NULL) ? TRUE : > > FALSE; > > just > > is_cd = (spice_usb_backend_device_get_libdev(bdev) == NULL); > OK > Also the name of the function is wrong, you are not checking if the device > is a shared cd but if is a emulated device. > > Well, I don't think the name spice_usb_device_manager_is_device_shared_cd is wrong. It is an API function exposed to the external modules, and, just as the name suggests, it promises to return True iff the device is a shared CD. The implementation is based on the (currently true) fact, that only CDs have libdev==NULL. It may seem awkward, but the alternative is to maintain an "is_cd" flag. It is possible of course, but I opted for a minimal amount of changes in structures and preferred to use existing functions even if their semantics may seem irrelevant :) > > @@ -1499,6 +1587,7 @@ gboolean spice_usb_device_is_isochronous(const > > SpiceUsbDevice *info) > > return spice_usb_backend_device_isoch((SpiceUsbBackendDevice*) > info); > > } > > > > + > > spurious hunk, remove > Of course!
_______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
