[Spice-devel] [PATCH v2 2/2] usb-device-{manager, widget}: Remove misleading message when there are no channels to redirect

[Jonathon Jongsma]

On Thu, 2016-01-21 at 19:09 +0100, Fabiano Fidêncio wrote:
> The whole code from usb-device-manager is quite a mess as pointed out by

Perhaps you could use a slightly gentler description :)

> Jonathon during the first round of code review [0].
> Keeping this in mind and knowing that that code needs a refactor, I'm
> proposing a simple (and messy) fix that can be backported easily before
> actually dig into a code refactoring task.
> The solution is basically adding a new error and using it to filter out
> this specific kind out situation, avoiding to show that misleading
> warning message when the last usb redirection channel is taken.
> 
> [0]:
> http://lists.freedesktop.org/archives/spice-devel/2016-January/025864.html
> 
> Related: rhbz#1298772
> ---
>  src/spice-client.h       | 1 +
>  src/usb-device-manager.c | 4 +++-
>  src/usb-device-widget.c  | 6 ++++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/spice-client.h b/src/spice-client.h
> index 32b79ea..ea7a557 100644
> --- a/src/spice-client.h
> +++ b/src/spice-client.h
> @@ -82,6 +82,7 @@ typedef enum
>      SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME,
>      SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
>      SPICE_CLIENT_ERROR_USB_SERVICE,
> +    SPICE_CLIENT_ERROR_NO_USB_CHANNELS_AVAILABLE,
>  } SpiceClientError;
>  
>  #ifndef SPICE_DISABLE_DEPRECATED
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 9e0e785..f40cc67 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -1701,7 +1701,9 @@
> spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager  *self,
>              break;
>      }
>      if (i == priv->channels->len) {
> -        g_set_error_literal(err, SPICE_CLIENT_ERROR,
> SPICE_CLIENT_ERROR_FAILED,
> +        g_set_error_literal(err,
> +                            SPICE_CLIENT_ERROR,
> +                            SPICE_CLIENT_ERROR_NO_USB_CHANNELS_AVAILABLE,
>                              _("There are no free USB channels"));
>          return FALSE;
>      }
> diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
> index 0866adb..544ceaa 100644
> --- a/src/usb-device-widget.c
> +++ b/src/usb-device-widget.c
> @@ -386,6 +386,11 @@ static void check_can_redirect(GtkWidget *widget,
> gpointer user_data)
>                                                                  device,
> &err);
>      gtk_widget_set_sensitive(widget, can_redirect);
>  
> +    if (g_error_matches(err,
> +                        SPICE_CLIENT_ERROR,
> +                        SPICE_CLIENT_ERROR_NO_USB_CHANNELS_AVAILABLE))
> +        goto end;
> +
>      /* If we cannot redirect this device, append the error message to
>         err_msg, but only if it is *not* already there! */
>      if (!can_redirect) {
> @@ -402,6 +407,7 @@ static void check_can_redirect(GtkWidget *widget, gpointer
> user_data)
>          }
>      }
>  
> +end:
>      g_clear_error(&err);
>  }
>  


As a downstream patch, I think this is perfectly fine. I'm not convinced that we
should commit it upstream though. What do you think?

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>