[Spice-devel] [spice-gtk v2 1/1] usb-redirection: implementation of usb backend layer
Yuri Benditovich
yuri.benditovich at daynix.com
Mon Jan 21 13:31:28 UTC 2019
On Tue, Jan 15, 2019 at 4:52 PM Christophe Fergeau <cfergeau at redhat.com>
wrote:
> 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.
>
No, I mean that this is the same and there is no difference between
ref/unref
and acquire/release; ref/unref is used in glib, so I prefer to use
acquire/release.
I do not have a goal to mimic libusb, I rather prefer to remove any
knowledge
about libusb from all the parts of spice-gtk except of backend code.
I've asked whether this change is mandatory, i.e. if this leads the
rejection of the patch.
Please respond, preferrably yes or no.
>
> > > > @@ -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.
>
>
Changing the code as you suggest makes further patches more complicated.
Please let me know whether this is a requirement.
> >
> > >
> > > > + 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...
>
>
Please let me know whether this change is mandatory.
> > > > +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?
>
>
If I would change the name of the structure in usb-device-manager in my
initial commit,
I'd immediately asked NOT to do changes that are not related to current
commit.
Again, I try to make this patch more simple.
Additional changes (NOT related directly to this patch) IMO can be done
later.
make this patch more complicated.
Please let me know whether this is mandatory.
>
> > > > +
> > > > +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'.
>
>
New type (public one) is introduced because the usb-device-manager uses
private data structure to the same, so the backed can't just reuse it.
Usb-device-manager will be able use this public type, if it will be
required at all.
>From my point of view, further simplification of the usb-device-manager will
remove the need to have any separate data structure.
> Christophe
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190121/c2c65075/attachment-0001.html>
More information about the Spice-devel
mailing list