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

[Victor Toso]

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.

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
-------------- 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/20190416/42ee8e2c/attachment.sig>