Re: [spice-gtk v1 2/4] FIXUP suggestion: drop the function instead

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 21, 2019 at 10:53:22AM +0000, Victor Toso wrote:
> Hi,
> 
> On Tue, May 21, 2019 at 05:22:39AM -0400, Frediano Ziglio wrote:
> > > 
> > > From: Victor Toso <me@xxxxxxxxxxxxxx>
> > > 
> > > In this patch series, spice_usbutil_libusb_strerror() becomes useless
> > > in favor of libusb_error_name(). Instead of making the helper function
> > > to call the native one, drop the usage of the helper function
> > > entirely.
> > > 
> > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > 
> > Are we sure we want libusb_error_name and not libusb_strerror ?
> >
> > I think (not tested) libusb_error_name give more a "C" version
> > of the error, not a string to show to the user.
> 
> Ah, indeed.
> 
> https://github.com/libusb/libusb/blob/master/libusb/core.c#L2682
> 
> We might propose a better api to libusb instead of dropping this
> function here.

https://github.com/libusb/libusb/issues/573

> Weird that those errors are not i18n with _(""); 
> 
> That might need to be fixed (...)
> 
> > > ---
> > >  src/channel-usbredir.c   | 2 +-
> > >  src/usb-device-manager.c | 8 ++++----
> > >  src/usbutil.c            | 6 ------
> > >  src/usbutil.h            | 1 -
> > >  src/win-usb-dev.c        | 4 ++--
> > >  5 files changed, 7 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > > index 1910ff6..079a976 100644
> > > --- a/src/channel-usbredir.c
> > > +++ b/src/channel-usbredir.c
> > > @@ -292,7 +292,7 @@ static gboolean spice_usbredir_channel_open_device(
> > >      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);
> > > +                    libusb_error_name(rc), rc);
> > >          return FALSE;
> > >      }
> > >  
> > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > index bd42142..c1df977 100644
> > > --- a/src/usb-device-manager.c
> > > +++ b/src/usb-device-manager.c
> > > @@ -281,7 +281,7 @@ static gboolean
> > > spice_usb_device_manager_initable_init(GInitable  *initable,
> > >      /* Initialize libusb */
> > >      rc = libusb_init(&priv->context);
> > >      if (rc < 0) {
> > > -        const char *desc = spice_usbutil_libusb_strerror(rc);
> > > +        const char *desc = libusb_error_name(rc);
> > >          g_warning("Error initializing USB support: %s [%i]", desc, rc);
> > >          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > >                      "Error initializing USB support: %s [%i]", desc, rc);
> > > @@ -308,7 +308,7 @@ static gboolean
> > > spice_usb_device_manager_initable_init(GInitable  *initable,
> > >          LIBUSB_HOTPLUG_MATCH_ANY, LIBUSB_HOTPLUG_MATCH_ANY,
> > >          spice_usb_device_manager_hotplug_cb, self, &priv->hp_handle);
> > >      if (rc < 0) {
> > > -        const char *desc = spice_usbutil_libusb_strerror(rc);
> > > +        const char *desc = libusb_error_name(rc);
> > >          g_warning("Error initializing USB hotplug support: %s [%i]", desc,
> > >          rc);
> > >          g_set_error(err, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> > >                    "Error initializing USB hotplug support: %s [%i]", desc,
> > >                    rc);
> > > @@ -730,7 +730,7 @@ static gboolean
> > > spice_usb_device_manager_get_device_descriptor(
> > >  
> > >          bus = libusb_get_bus_number(libdev);
> > >          addr = libusb_get_device_address(libdev);
> > > -        errstr = spice_usbutil_libusb_strerror(errcode);
> > > +        errstr = libusb_error_name(errcode);
> > >          g_warning("cannot get device descriptor for (%p) %d.%d -- %s(%d)",
> > >                    libdev, bus, addr, errstr, errcode);
> > >          return FALSE;
> > > @@ -1068,7 +1068,7 @@ static gpointer
> > > spice_usb_device_manager_usb_ev_thread(gpointer user_data)
> > >      while (g_atomic_int_get(&priv->event_thread_run)) {
> > >          rc = libusb_handle_events(priv->context);
> > >          if (rc && rc != LIBUSB_ERROR_INTERRUPTED) {
> > > -            const char *desc = spice_usbutil_libusb_strerror(rc);
> > > +            const char *desc = libusb_error_name(rc);
> > >              g_warning("Error handling USB events: %s [%i]", desc, rc);
> > >              break;
> > >          }
> > > diff --git a/src/usbutil.c b/src/usbutil.c
> > > index 4aa6ef7..5052ef3 100644
> > > --- a/src/usbutil.c
> > > +++ b/src/usbutil.c
> > > @@ -58,12 +58,6 @@ static GMutex usbids_load_mutex;
> > >  static int usbids_vendor_count = 0; /* < 0: failed, 0: empty, > 0: loaded */
> > >  static usb_vendor_info *usbids_vendor_info = NULL;
> > >  
> > > -G_GNUC_INTERNAL
> > > -const char *spice_usbutil_libusb_strerror(enum libusb_error error_code)
> > > -{
> > > -    return libusb_error_name(error_code);
> > > -}
> > > -
> > >  #ifdef __linux__
> > >  /* <Sigh> libusb does not allow getting the manufacturer and product strings
> > >     without opening the device, so grab them directly from sysfs */
> > > diff --git a/src/usbutil.h b/src/usbutil.h
> > > index de5e92a..50e3949 100644
> > > --- a/src/usbutil.h
> > > +++ b/src/usbutil.h
> > > @@ -28,7 +28,6 @@
> > >  
> > >  G_BEGIN_DECLS
> > >  
> > > -const char *spice_usbutil_libusb_strerror(enum libusb_error error_code);
> > >  void spice_usb_util_get_device_strings(int bus, int address,
> > >                                         int vendor_id, int product_id,
> > >                                         gchar **manufacturer, gchar
> > >                                         **product);
> > > diff --git a/src/win-usb-dev.c b/src/win-usb-dev.c
> > > index a4dfa78..ce599b9 100644
> > > --- a/src/win-usb-dev.c
> > > +++ b/src/win-usb-dev.c
> > > @@ -113,7 +113,7 @@ g_udev_client_list_devices(GUdevClient *self, GList
> > > **devs,
> > >  
> > >      rc = libusb_get_device_list(priv->ctx, &lusb_list);
> > >      if (rc < 0) {
> > > -        const char *errstr = spice_usbutil_libusb_strerror(rc);
> > > +        const char *errstr = libusb_error_name(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"]",
> > > @@ -170,7 +170,7 @@ g_udev_client_initable_init(GInitable *initable,
> > > GCancellable *cancellable,
> > >  
> > >      rc = libusb_init(&priv->ctx);
> > >      if (rc < 0) {
> > > -        const char *errstr = spice_usbutil_libusb_strerror(rc);
> > > +        const char *errstr = libusb_error_name(rc);
> > >          g_warning("Error initializing USB support: %s [%i]", errstr, rc);
> > >          g_set_error(err, G_UDEV_CLIENT_ERROR, G_UDEV_CLIENT_LIBUSB_FAILED,
> > >                      "Error initializing USB support: %s [%i]", errstr, rc);
> > 
> > Frediano


Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]