[Spice-devel] [PATCH 2/9] add spice_usb_device_manager shared CD related api functions

[Alexander Nezhinsky]

On Fri, Dec 6, 2019 at 12:04 PM Frediano Ziglio <fziglio at 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!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20191207/4c2a218a/attachment.html>