[Spice-devel] [PATCH 1/3] usb: use native libusb procedure for getting error name

[Victor Toso]

Hi,

On Tue, Apr 16, 2019 at 08:51:52AM +0000, Victor Toso wrote:
> Hi,
> 
> On Mon, Apr 15, 2019 at 03:42:05PM +0300, Yuri Benditovich wrote:
> > IIUC, what you call 'simpler' is:
> > - making unneeded changes in several files (instead of one)
> > - in the next patch remove these changes completely
> > 
> > Did I miss something?
> 
> Current patch series changes spice_usbutil_libusb_strerror(),
> then drops its usage and then a new patch to remove
> spice_usbutil_libusb_strerror() is needed.
> 
> My suggestion was to remove spice_usbutil_libusb_strerror() as
> first thing, because you can replace that with
> libusb_error_name().
> 
> From the repository POV, this is nicer IMHO. Yes, you would need
> to rebase your branch.
> 
> Note that this is a friendly review, if you disagree feel free to
> say so and why. I'm trying to make my rationale clear above so
> you can just clarify why my suggestion is bad, if you think so.
> 
> I'll be looking further to the other patches Today, sorry for
> taking some time on it.

Yes, taking more time that I anticipated. The whole USB stack is
new to me and that makes a good opportunity to understand more
and more things which delays a bit my review here.

Replying here because I did a FIXUP patch that removes the
function as suggested and the following up patches are almost
untouched, see the branch yuri-ml-usb-backend-layer [0] for the
patch + rebase.

[0] https://gitlab.freedesktop.org/victortoso/spice-gtk/commits/yuri-ml-usb-backend-layer

One other thing that bothered me was the #ifdef USE_POLKIT on
channel-usbredir which I proposed to remove. I've rebased your
code on top of that as well, needs a small FIXUP patch it seems.

[1] https://gitlab.freedesktop.org/victortoso/spice-gtk/commits/yuri-usb-backend-on-polkit-branch

I'll be providing you better feedback later this week.

Cheers,

> Cheers,
> Victor
> 
> > On Mon, Apr 15, 2019 at 3:18 PM Christophe Fergeau <cfergeau at redhat.com> wrote:
> > >
> > > On Thu, Apr 11, 2019 at 12:37:17PM +0000, Victor Toso wrote:
> > > > Hi,
> > > >
> > > > On Thu, Apr 11, 2019 at 02:57:21PM +0300, Yuri Benditovich wrote:
> > > > > On Thu, Apr 11, 2019 at 12:35 PM Victor Toso <victortoso at redhat.com> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Wed, Apr 10, 2019 at 10:31:37PM +0300, Yuri Benditovich wrote:
> > > > > > > libusb has libusb_error_name procedure that returns name
> > > > > > > for any error that libusb may return, so we do not need
> > > > > > > to analyze error values by ourselves.
> > > > > > >
> > > > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich at daynix.com>
> > > > > >
> > > > > > Before applying the series:
> > > > > >
> > > > > > (master 15e06ead) $ grepi "spice_usbutil_libusb_strerror" src/
> > > > > > src/win-usb-dev.c:116:        const char *errstr = spice_usbutil_libusb_strerror(rc);
> > > > > > src/win-usb-dev.c:173:        const char *errstr = spice_usbutil_libusb_strerror(rc);
> > > > > > src/channel-usbredir.c:312: spice_usbutil_libusb_strerror(rc), rc);
> > > > > > src/usbutil.c:62:const char *spice_usbutil_libusb_strerror(enum libusb_error error_code)
> > > > > > src/usbutil.h:31:const char *spice_usbutil_libusb_strerror(enum libusb_error error_code);
> > > > > > src/usb-device-manager.c:284:        const char *desc = spice_usbutil_libusb_strerror(rc);
> > > > > > src/usb-device-manager.c:311:        const char *desc = spice_usbutil_libusb_strerror(rc);
> > > > > > src/usb-device-manager.c:733:        errstr = spice_usbutil_libusb_strerror(errcode);
> > > > > > src/usb-device-manager.c:1071:            const char *desc = spice_usbutil_libusb_strerror(rc);
> > > > > >
> > > > > > After applying the series:
> > > > > > (yuri-usb-b-layers-v1 5f87d90d) $ grepi "spice_usbutil_libusb_strerror" src/
> > > > > > (yuri-usb-b-layers-v1 5f87d90d) $
> > > > > >
> > > > > > So, I think it makes sense to use this patch to drop this
> > > > > > function and always use libusb_error_name() instead, agree?
> > > > >
> > > > > Finally, this series drops this functions and uses libusb_error_name.
> > > >
> > > > Yes,
> > > >
> > > > > It was possible to drop this function in the first patch, but
> > > > > this would not make too much sense ( as all these new calls to
> > > > > libusb_error_name() would be removed due to isolation of
> > > > > libusb).
> > > >
> > > > For me it makes sense because I know that this function can be
> > > > dropped now even if later patches would change the code path of
> > > > callers of spice_usbutil_libusb_strerror/libusb_error_name again.
> > > >
> > > > That is, removing this function as first patch would introduce no
> > > > regression and cleanup the code a bit. One patch less in the
> > > > queue and could be merged before the others ;)
> > >
> > > Yep, makes sense to me too to start by making the code simpler, and
> > > merging the preparatory patches early.
> > >
> > > Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

-------------- 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/20190507/90e6f282/attachment-0001.sig>