On Fri, Dec 6, 2019 at 12:04 PM Frediano Ziglio <fziglio@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel