[Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
Christophe Fergeau
cfergeau at redhat.com
Tue Jan 15 14:52:09 UTC 2019
Hey,
On Mon, Jan 07, 2019 at 03:50:12PM +0200, Yuri Benditovich wrote:
> >
> > > if (!priv->host)
> > > - g_error("Out of memory allocating usbredirhost");
> > > + g_error("Out of memory initializing redirection support");
> > >
> > > -#if USBREDIR_VERSION >= 0x000701
> > > - usbredirhost_set_buffered_output_size_cb(priv->host,
> > usbredir_buffered_output_size_callback);
> > > -#endif
> > > #ifdef USE_LZ4
> > > spice_channel_set_capability(channel,
> > SPICE_SPICEVMC_CAP_DATA_COMPRESS_LZ4);
> > > #endif
> > > @@ -299,8 +279,6 @@ static gboolean spice_usbredir_channel_open_device(
> > > {
> > > SpiceUsbredirChannelPrivate *priv = channel->priv;
> > > SpiceSession *session;
> > > - libusb_device_handle *handle = NULL;
> > > - int rc, status;
> > > SpiceUsbDeviceManager *manager;
> > >
> > > g_return_val_if_fail(priv->state == STATE_DISCONNECTED
> > > @@ -309,21 +287,16 @@ static gboolean spice_usbredir_channel_open_device(
> > > #endif
> > > , FALSE);
> > >
> > > - rc = libusb_open(priv->device, &handle);
> > > - if (rc != 0) {
> > > - g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > > - "Could not open usb device: %s [%i]",
> > > - spice_usbutil_libusb_strerror(rc), rc);
> > > - return FALSE;
> > > - }
> > > -
> > > priv->catch_error = err;
> > > - status = usbredirhost_set_device(priv->host, handle);
> > > - priv->catch_error = NULL;
> > > - if (status != usb_redir_success) {
> > > - g_return_val_if_fail(err == NULL || *err != NULL, FALSE);
> > > + if (!spice_usb_backend_channel_attach(priv->host, priv->device,
> > err)) {
> > > + priv->catch_error = NULL;
> > > + if (*err == NULL) {
> > > + g_set_error(err, SPICE_CLIENT_ERROR,
> > SPICE_CLIENT_ERROR_FAILED,
> > > + "Error attaching device: (no error information)");
> >
> > You could line up the "Error .." with the opening parenthesis.
> >
> >
> This is not described exactly in Spice coding style document and existing
> code of spice-gtk
> uses different solution for indentation, so this is point of personal
> preference.
From a quick look, aligning multiple line arguments lists seem much more
common that then opposite. See the g_set_error call that this hunk is
removing for example.
> > > @@ -362,8 +335,7 @@ static void spice_usbredir_channel_open_acl_cb(
> > > spice_usbredir_channel_open_device(channel, &err);
> > > }
> > > if (err) {
> > > - libusb_unref_device(priv->device);
> > > - priv->device = NULL;
> > > + g_clear_pointer(&priv->device,
> > spice_usb_backend_device_release);
> >
> > _unref rather than _release?
> >
>
> This can be changed in V3, although I would prefer not to change it.
> Semantics of ref/unref and acquire/release are not different.
> Please let me know whether this change in mandatory.
I assume you mean that the semantics of ref/unref and acquire/release
*are* different? If this is what you mean, how are they different?
If the semantics are the same, then let's use the same name.
> > > @@ -446,7 +420,8 @@ void spice_usbredir_channel_connect_device_async(
> > > goto done;
> > > }
> > >
> > > - priv->device = libusb_ref_device(device);
> > > + spice_usb_backend_device_acquire(device);
> > > + priv->device = device;
> >
> > You could mimic libusb API actually,
> > priv->device = spice_usb_backend_device_ref(device);
> >
>
> I do not think I need to mimic libusb or something other.
> Please let me know whether this change is mandatory.
libusb_ref_device, g_object_ref all return a pointer to the new ref, and
this makes that code slightly simpler, so just a suggestion in case you
think that's a good change.
> >
> > > + void *msc;
> > > + } d;
> > > + gboolean is_libusb;
> > > + gint ref_count;
> > > + SpiceUsbBackendChannel *attached_to;
> >
> > You don't need 'msc' just yet, nor 'is_libusb', and I'm not sure about
> > 'attached_to'
> >
>
> msc can be changed to reserved if you do not like it so much.
> removal of is_libusb makes further merges harder (and current commit is
> only prerequisite for further merge).
> attached_to is useful for debugging/support
> Please let me know whether this is a requirement.
This patch is refactoring the existing code so that we can add more
stuff on top of it. It should not be adding new things, especially if
they are not used in the current commit. attached_to can be added in a
separate commit with the debugging checks that it lets us do, msc can be
added when it's needed.
>
> >
> > > + void *hotplug_user_data;
> > > + libusb_hotplug_callback_handle hotplug_handle;
> > > +};
> > > +
> > > +struct _SpiceUsbBackendChannel
> > > +{
> > > + struct usbredirhost *usbredirhost;
> > > + uint8_t *read_buf;
> > > + int read_buf_size;
> > > + struct usbredirfilter_rule *rules;
> > > + int rules_count;
> > > + SpiceUsbBackendDevice *attached;
> >
> > I don't think this is used.
> >
>
> The same as is_libusb.
> In general even now the reference to attached device and attached channel
> are useful for debugging and parsing logs and dumps.
If it's not used in this commit, then it does not belong in it. Once it
starts being used, then we can add it. Or if it's a debugging feature,
just add it in its own commit with a description of why we want it.
> >
> > > + SpiceUsbBackendChannelInitData channel_data;
> >
> > I don't think 'channel_data' is a much more descriptive than the
> > previously used 'data'. SpiceUsbBackendChannelInitData is a bunch of
> > vfuncs/callbacks, so maybe use one of these words in the naming?
> >
>
> It can be renamed in V3, please provide the name you agree with.
backend_vfuncs maybe? Overall, this just feels like something odd we put
in each SpiceUsbBackendChannel instances for lack of a good place in the
design.
>
>
> >
> > > +};
> > > +
> > > +// it's unclear why we use this procedure instead of libusb_error_name,
> > > +// which by definition supports any error that libusb returns
> >
> > Why not add a commit before this one switching libusb_error_name() then?
> >
>
> Because this looks strange: first change it in 10 places then remove all of
> them in next commit.
> Also because this make the merge very difficult (for me).
On your cd redirection branch, do a git rebase -i origin/master^ and
say that you want to edit the commit for the current git master. Do the
libusb_error_name() change there, commit it.
git revert HEAD
git rebase --continue
Then you rebase once more to do squash the revert with this commit, and
you need to edit this commit to make it use libusb_error_name() too.
> I can remove this note. I can use libusb_error_name instead of this
> procedure.
If what I described above is complicated, then remove the comment from
this commit, and you can have a followup commit switching to
libusb_error_name().
> >
> > > +#endif
> > > +#endif
> > > + }
> > > + SPICE_DEBUG("%s <<", __FUNCTION__);
> > > + return be;
> > > +}
> > > +
> > > +gboolean spice_usb_backend_handle_events(SpiceUsbBackend *be)
> > > +{
> > > + SPICE_DEBUG("%s >>", __FUNCTION__);
> > > + gboolean ok = FALSE;
> > > + if (be->libusb_context) {
> > > + SPICE_DEBUG("%s >> libusb", __FUNCTION__);
> > > + int res = libusb_handle_events(be->libusb_context);
> > > + ok = res == 0;
> > > + if (res && res != LIBUSB_ERROR_INTERRUPTED) {
> > > + const char *desc = spice_usbutil_libusb_strerror(res);
> > > + g_warning("Error handling USB events: %s [%i]", desc, res);
> > > + ok = FALSE;
> >
> > You don't really need 'ok' in that method, just return FALSE; here...
> >
>
> Please let me know whether this change is mandatory.
>
>
> >
> > > + }
> > > + }
> > > + SPICE_DEBUG("%s << %s %d", __FUNCTION__,
> > > + be->libusb_context ? "libusb" : "no libusb", ok);
> > > + return ok;
> >
> > ... and return TRUE; there (and I'm not sure how useful the SPICE_DEBUG
> > are going to be?)
> >
> >
> Please let me know whether this change is mandatory.
> Reformatting the code will make further merger more difficult.
Complaining about merge conflicts related to a 10 line function is not a
very compelling argument against making code simpler/easier to read in a
preliminary patch...
> > > +gboolean spice_usb_backend_handle_hotplug(SpiceUsbBackend *be,
> > > + void *user_data,
> > > + usb_hot_plug_callback proc)
> >
> > Why not
> > spice_usb_backend_enable_hotplug/spice_usb_backend_disable_hotplug?
> > (and now that we have a proper SpiceUsbBackend struct wrapping the
> > libusb stuff, I wonder if it would not make sense to just turn it into a
> > gobject, and emit signals when we get new devices).
> >
> >
> Regarding 'why not to split it to two procedures': this is point of
> personal preference. If the splitting is mandatory, please let me
> know, I'll do that in V3.
Well, this function is not handling anything, it's setting up a handler
for future hotplug events. So in my opinion, the naming is misleading.
> Regarding turning it to GObject - this will discard entire patch and spawn
> completely different activity that requires additional debugging. Is
> this is mandatory, we will need to reevaluate our ability to deliver
> the patches.
I agree this is a matter that would have been easier to solve
during the initial design/while writing the code.
> > > +SpiceUsbBackendDevice
> > **spice_usb_backend_get_device_list(SpiceUsbBackend *be)
> > > +{
> > > + LOUD_DEBUG("%s >>", __FUNCTION__);
> > > + libusb_device **devlist = NULL, **dev;
> > > + SpiceUsbBackendDevice *d, **list;
> > > +
> > > + int n = 0, index;
> > > +
> > > + if (be && be->libusb_context) {
> > > + libusb_get_device_list(be->libusb_context, &devlist);
> > > + }
> > > +
> > > + // add all the libusb device that not present in our list
> > > + for (dev = devlist; dev && *dev; dev++) {
> > > + n++;
> > > + }
> > > +
> > > + list = g_new0(SpiceUsbBackendDevice*, n + 1);
> > > +
> > > + index = 0;
> > > +
> > > + for (dev = devlist; dev && *dev; dev++) {
> > > + d = allocate_backend_device(*dev);
> > > + if (!d) {
> > > + libusb_unref_device(*dev);
> > > + } else {
> > > + SPICE_DEBUG("created dev %p, usblib dev %p", d, *dev);
> > > + list[index++] = d;
> > > + }
> > > + }
> > > +
> > > + if (devlist) {
> > > + libusb_free_device_list(devlist, 0);
> > > + }
> > > +
> > > + LOUD_DEBUG("%s <<", __FUNCTION__);
> > > + return list;
> > > +}
> >
> > I still think this could be made slightly simpler with GArray/GPtrArray.
> >
>
> I do not see any advantage at all, just additional change that makes
> further merges more complicated.
This makes the method a bit shorter, and avoids the bookkeeping related
to list length and position in the list:
SpiceUsbBackendDevice **spice_usb_backend_get_device_list(SpiceUsbBackend *be)
{
LOUD_DEBUG("%s >>", __FUNCTION__);
libusb_device **devlist = NULL;
libusb_device **dev;
GPtrArray *devices = g_ptr_array_new();
if (be && be->libusb_context) {
libusb_get_device_list(be->libusb_context, &devlist);
}
for (dev = devlist; dev && *dev; dev++) {
SpiceUsbBackendDevice *d;
d = allocate_backend_device(*dev);
if (!d) {
libusb_unref_device(*dev);
} else {
SPICE_DEBUG("created dev %p, usblib dev %p", d, *dev);
g_ptr_array_add(devices, d);
}
}
g_ptr_array_add(devices, NULL);
if (devlist) {
libusb_free_device_list(devlist, 0);
}
LOUD_DEBUG("%s <<", __FUNCTION__);
return (SpiceUsbBackendDevice **)g_ptr_array_free(devices, FALSE);
}
> > > +SpiceUsbBackendChannel
> > *spice_usb_backend_channel_initialize(SpiceUsbBackend *be,
> > > + const
> > SpiceUsbBackendChannelInitData *init_data)
> > > +{
> > > + SpiceUsbBackendChannel *ch = g_new0(SpiceUsbBackendChannel, 1);
> > > + SPICE_DEBUG("%s >>", __FUNCTION__);
> > > + ch->channel_data = *init_data;
> > > + if (be->libusb_context) {
> >
> > You probably can just have g_return_val_if_fail(be->libusb_context !=
> > NULL, NULL) at the very beginning of that method,
> > and only do the allocation after that.
> >
>
> Please let me know whether this is mandatory.
> I can do that but then later it should be rewritten again.
As your prefer.
> > > +typedef struct UsbDeviceInformation
> > > +{
> > > + uint16_t bus;
> > > + uint16_t address;
> > > + uint16_t vid;
> > > + uint16_t pid;
> > > + uint8_t class;
> > > + uint8_t subclass;
> > > + uint8_t protocol;
> > > + uint8_t isochronous;
> > > +} UsbDeviceInformation;
> >
> > As already mentioned in the past, please let's not duplicate very similar
> > information with different names/different types in 2 structures with
> > very close names (UsbDeviceInformation and SpiceUsbDeviceInfo)
> >
> >
> I do not see any possibility to do that in context of current commit.
> This can be done later when SpiceUsbDevice will have persistent link
> to backend device with it's information. For now this information is not
> duplicated, it is cached in SpiceUsbDeviceInfo and can be returned
> without looking for backend device.
> If this is mandatory requirement, please let me know - this discards
> current commit and we will need to reevaluate our plans.
I think you are explaining why you cannot have a single instance of that
data in memory. I don't really mind if we have multiple instances of
that data in memory, however having 2 different types being used to
store what is essentially the same thing is what I'm objecting to.
How complicated would it be to use the same type each time we need to
store USB information?
> > > +
> > > +typedef void (*usb_channel_error_callback)(void *user_data, const char
> > *msg);
> > > +typedef int (*usb_channel_write_callback)(void *user_data, uint8_t
> > *data, int count);
> > > +typedef gboolean (*usb_channel_is_ready_callback)(void *user_data);
> > > +typedef uint64_t (*usb_channel_get_queue_size)(void *user_data);
> > > +
> > > +typedef struct SpiceUsbBackendChannelInitData
> > > +{
> > > + void *user_data;
> > > + usb_channel_error_callback on_error;
> > > + usb_channel_write_callback write_callback;
> > > + usb_channel_is_ready_callback is_channel_ready;
> > > + usb_channel_get_queue_size get_queue_size;
> >
> > I would not add typedef for these vfuncs, I think the types are only
> > used here, so I'd directly use:
> > void *user_data;
> > void (*on_error)(void *user_data, const char *msg);
> > ...
> >
> >
> Please let me know whether this is mandatory.
Please change it, unless you plan to make extensive use of these types
later on.
> > > @@ -122,8 +124,7 @@ static ssize_t
> > > g_udev_client_list_devices(GUdevClient *self, GList **devs,
> > > GError **err, const gchar *name)
> > > {
> > > - gssize rc;
> > > - libusb_device **lusb_list, **dev;
> > > + SpiceUsbBackendDevice **lusb_list, **dev;
> > > GUdevClientPrivate *priv;
> > > GUdevDeviceInfo *udevinfo;
> > > GUdevDevice *udevice;
> > > @@ -136,13 +137,8 @@ g_udev_client_list_devices(GUdevClient *self, GList
> > **devs,
> > >
> > > g_return_val_if_fail(self->priv->ctx != NULL, -3);
> > >
> > > - rc = libusb_get_device_list(priv->ctx, &lusb_list);
> > > - if (rc < 0) {
> > > - const char *errstr = spice_usbutil_libusb_strerror(rc);
> > > - g_warning("%s: libusb_get_device_list failed - %s", name,
> > errstr);
> > > - g_set_error(err, G_UDEV_CLIENT_ERROR,
> > G_UDEV_CLIENT_LIBUSB_FAILED,
> > > - "%s: Error getting device list from libusb: %s
> > [%"G_GSSIZE_FORMAT"]",
> > > - name, errstr, rc);
> > > + lusb_list = spice_usb_backend_get_device_list(priv->ctx);
> > > + if (!lusb_list) {
> > > return -4;
> > > }
> >
> > Why did you remove the g_set_error?
> >
> >
> Possible, just because this never happens, empty list it will receive
> anyway.
> I can return it back, if you like it.
Hmm, should spice_usb_backend_get_device_list() be able to return errors
such as what was done before? I think before we could differentiate
between "empty list of usb devices" and "an error occurred while getting
the list", while now all we know is that we got an empty list. But we
still return -4 on an empty list, which I assume indicates an error?
So why not stop setting the GError?
> Please respond regarding 2 serious questions:
> - whether there is change in indirection level or wrong use of obtained
> libdev
I'll answer this in a separate email, I did not have time to consider it
yet.
> - whether we discard this commit to avoid caching of device properties
> If we do not discard it, please let me know whether specific mentioned
> changes are mandatory.
and as indicated before, I think there was a misunderstanding on this
one, what I object to is introducing a new type to hold USB device
information, I don't necessarily object to the additional 'caching'.
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190115/87e0159a/attachment-0001.sig>
More information about the Spice-devel
mailing list