[2/2] usb-device-manager: Don't log critical on lacking UsbDk

Submitted by Victor Toso on Aug. 1, 2018, 2:57 p.m.

Details

Message ID 20180801145747.15281-2-victortoso@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Aug. 1, 2018, 2:57 p.m.
From: Victor Toso <me@victortoso.com>

The lack of UsbDk is logged with messages like:

 | GSpice-WARNING **: Error initializing USB support: Entity not found [-5]
 | Spice-DEBUG: usb-device-manager.c:272:spice_usb_device_manager_init:
 |              UsbDk driver is not installed

We don't need to log a critical for every check on usbdk_api handle
that might be done. That's really not necessary for
_usbdk_hider_clear() and debug message on _usbdk_hider_update() should
be enough to track bugs when UsbDk should be working.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/usb-device-manager.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
index 55bf67e..5e0469b 100644
--- a/src/usb-device-manager.c
+++ b/src/usb-device-manager.c
@@ -1932,13 +1932,13 @@  void _usbdk_hider_clear(SpiceUsbDeviceManager *manager)
 {
     SpiceUsbDeviceManagerPrivate *priv = manager->priv;
 
-    g_return_if_fail(priv->usbdk_api != NULL);
-
-    if (priv->usbdk_hider_handle != NULL) {
-        usbdk_clear_hide_rules(priv->usbdk_api, priv->usbdk_hider_handle);
-        usbdk_close_hider_handle(priv->usbdk_api, priv->usbdk_hider_handle);
-        priv->usbdk_hider_handle = NULL;
+    if (priv->usbdk_api == NULL || priv->usbdk_hider_handle == NULL) {
+        return;
     }
+
+    usbdk_clear_hide_rules(priv->usbdk_api, priv->usbdk_hider_handle);
+    usbdk_close_hider_handle(priv->usbdk_api, priv->usbdk_hider_handle);
+    priv->usbdk_hider_handle = NULL;
 }
 
 static
@@ -1946,7 +1946,10 @@  void _usbdk_hider_update(SpiceUsbDeviceManager *manager)
 {
     SpiceUsbDeviceManagerPrivate *priv = manager->priv;
 
-    g_return_if_fail(priv->usbdk_api != NULL);
+    if (priv->usbdk_api == NULL) {
+        SPICE_DEBUG("UsbDk is not being used, hider setup can't be done");
+        return;
+    }
 
     if (priv->auto_connect_filter == NULL) {
         SPICE_DEBUG("No autoredirect rules, no hider setup needed");

Comments

On Wed, 2018-08-01 at 16:57 +0200, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> The lack of UsbDk is logged with messages like:
> 
>  | GSpice-WARNING **: Error initializing USB support: Entity not
> found [-5]
>  | Spice-DEBUG: usb-device-
> manager.c:272:spice_usb_device_manager_init:
>  |              UsbDk driver is not installed


This warning message doesn't seem to match the code you're changing
below? For example, if usbdk_api is null, I would expect that the
_usbdk_hider_clear() function would abort at the g_return_if_fail(priv-
>usbdk_api) line and produce a critical warning that is something like:

GLib-CRITICAL **: _usbdk_hider_clear: assertion 'priv->usbdi_api !=
NULL' failed

So if you were getting the warning above, I'm not sure how this patch
would change it? Maybe I'm missing something.



> 
> We don't need to log a critical for every check on usbdk_api handle
> that might be done. That's really not necessary for
> _usbdk_hider_clear() and debug message on _usbdk_hider_update()
> should
> be enough to track bugs when UsbDk should be working.
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/usb-device-manager.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> index 55bf67e..5e0469b 100644
> --- a/src/usb-device-manager.c
> +++ b/src/usb-device-manager.c
> @@ -1932,13 +1932,13 @@ void _usbdk_hider_clear(SpiceUsbDeviceManager
> *manager)
>  {
>      SpiceUsbDeviceManagerPrivate *priv = manager->priv;
>  
> -    g_return_if_fail(priv->usbdk_api != NULL);
> -
> -    if (priv->usbdk_hider_handle != NULL) {
> -        usbdk_clear_hide_rules(priv->usbdk_api, priv-
> >usbdk_hider_handle);
> -        usbdk_close_hider_handle(priv->usbdk_api, priv-
> >usbdk_hider_handle);
> -        priv->usbdk_hider_handle = NULL;
> +    if (priv->usbdk_api == NULL || priv->usbdk_hider_handle == NULL)
> {
> +        return;
>      }
> +
> +    usbdk_clear_hide_rules(priv->usbdk_api, priv-
> >usbdk_hider_handle);
> +    usbdk_close_hider_handle(priv->usbdk_api, priv-
> >usbdk_hider_handle);
> +    priv->usbdk_hider_handle = NULL;
>  }
>  
>  static
> @@ -1946,7 +1946,10 @@ void _usbdk_hider_update(SpiceUsbDeviceManager
> *manager)
>  {
>      SpiceUsbDeviceManagerPrivate *priv = manager->priv;
>  
> -    g_return_if_fail(priv->usbdk_api != NULL);
> +    if (priv->usbdk_api == NULL) {
> +        SPICE_DEBUG("UsbDk is not being used, hider setup can't be
> done");
> +        return;
> +    }
>  
>      if (priv->auto_connect_filter == NULL) {
>          SPICE_DEBUG("No autoredirect rules, no hider setup needed");
Hi,

On Wed, Aug 01, 2018 at 11:17:25AM -0500, Jonathon Jongsma wrote:
> On Wed, 2018-08-01 at 16:57 +0200, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > The lack of UsbDk is logged with messages like:
> > 
> >  | GSpice-WARNING **: Error initializing USB support: Entity not
> > found [-5]
> >  | Spice-DEBUG: usb-device-
> > manager.c:272:spice_usb_device_manager_init:
> >  |              UsbDk driver is not installed
> 
> 
> This warning message doesn't seem to match the code you're changing
> below? For example, if usbdk_api is null, I would expect that the
> _usbdk_hider_clear() function would abort at the g_return_if_fail(priv-
> >usbdk_api) line and produce a critical warning that is something like:
> 
> GLib-CRITICAL **: _usbdk_hider_clear: assertion 'priv->usbdi_api !=
> NULL' failed
> 
> So if you were getting the warning above, I'm not sure how this patch
> would change it? Maybe I'm missing something.

I wasn't clear enough, for sure. The main problem that this patch
tries to fix is the excess of critical messages that are
happening on remote-viewer while UsbDk is not installed.

So I proposed that, having the warnings/debug above, the
critical messages weren't necessary but I'm not so sure anymore.

I think we might need some extra check that tell us that usbdk is
not installed instead of removing the checks around
priv->usbdk_api. The checks around usbdk_api could be useful in a
real error-like situation, I guess.

I'll send a v2 later on, thanks for the review!

Cheers,

> > We don't need to log a critical for every check on usbdk_api handle
> > that might be done. That's really not necessary for
> > _usbdk_hider_clear() and debug message on _usbdk_hider_update()
> > should
> > be enough to track bugs when UsbDk should be working.
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/usb-device-manager.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > index 55bf67e..5e0469b 100644
> > --- a/src/usb-device-manager.c
> > +++ b/src/usb-device-manager.c
> > @@ -1932,13 +1932,13 @@ void _usbdk_hider_clear(SpiceUsbDeviceManager
> > *manager)
> >  {
> >      SpiceUsbDeviceManagerPrivate *priv = manager->priv;
> >  
> > -    g_return_if_fail(priv->usbdk_api != NULL);
> > -
> > -    if (priv->usbdk_hider_handle != NULL) {
> > -        usbdk_clear_hide_rules(priv->usbdk_api, priv-
> > >usbdk_hider_handle);
> > -        usbdk_close_hider_handle(priv->usbdk_api, priv-
> > >usbdk_hider_handle);
> > -        priv->usbdk_hider_handle = NULL;
> > +    if (priv->usbdk_api == NULL || priv->usbdk_hider_handle == NULL)
> > {
> > +        return;
> >      }
> > +
> > +    usbdk_clear_hide_rules(priv->usbdk_api, priv-
> > >usbdk_hider_handle);
> > +    usbdk_close_hider_handle(priv->usbdk_api, priv-
> > >usbdk_hider_handle);
> > +    priv->usbdk_hider_handle = NULL;
> >  }
> >  
> >  static
> > @@ -1946,7 +1946,10 @@ void _usbdk_hider_update(SpiceUsbDeviceManager
> > *manager)
> >  {
> >      SpiceUsbDeviceManagerPrivate *priv = manager->priv;
> >  
> > -    g_return_if_fail(priv->usbdk_api != NULL);
> > +    if (priv->usbdk_api == NULL) {
> > +        SPICE_DEBUG("UsbDk is not being used, hider setup can't be
> > done");
> > +        return;
> > +    }
> >  
> >      if (priv->auto_connect_filter == NULL) {
> >          SPICE_DEBUG("No autoredirect rules, no hider setup needed");