Re: [Spice-devel] [PATCH 1/3] usb: use native libusb procedure for getting error name
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
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
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
