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

2019-04-15 Thread Christophe Fergeau
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?

Ah I did not look at the next patches :) Since you intentionally keep
this wrapper because it's going to be shortlived, I'd explain it in the
commit log so that it's clear that it's intentional.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2019-04-15 Thread Yuri Benditovich
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?


On Mon, Apr 15, 2019 at 3:18 PM Christophe Fergeau  wrote:
>
> On Thu, Apr 11, 2019 at 12:37:17PM +, 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  
> > > 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 
> > > >
> > > > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2019-04-15 Thread Christophe Fergeau
On Thu, Apr 11, 2019 at 12:37:17PM +, 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  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 
> > >
> > > 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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel