[Spice-devel] [spice-gtk PATCH v1 1/1] channel-usbredir: better control of usbredir log level

[Marc-André Lureau]

Hi

----- Original Message -----
> Hi,
> 
> On Tue, Dec 01, 2015 at 07:19:58AM -0500, Marc-André Lureau wrote:
> > ----- Original Message -----
> > > Hi,
> > >
> > > On Tue, Dec 01, 2015 at 06:41:47AM -0500, Marc-André Lureau wrote:
> > > > ----- Original Message -----
> > > > > Hi
> > > > >
> > > > > ----- Original Message -----
> > > > > > usbredir on debug level is highly verbose. That does not help much
> > > > > > when
> > > > > > we are trying to debug the spice-gtk side of usbredir.
> > > > > >
> > > > > > This patch introduces SPICE_DEBUG_USBREDIR env var to set the log
> > > > > > level
> > > > > > for usbredir side. If this is not set, usbredir will behave the
> > > > > > same
> > > > > > way
> > > > > > as it was by checking SPICE_DEBUG env var.
> > > > > >
> > > > > > WARNING level is the default when neither env var are set.
> > > > >
> > > > > Why not filtering out the strings you don't want with -v ? In
> > > > > general,
> > > > > with
> > > > > the spice-gtk strings, you can easily strip down the log to what you
> > > > > need,
> > > > > and this is a lot more flexible than having various environment
> > > > > variables.
> > > >
> > > > I mean filtering with "grep -v"
> > >
> > > I don't understand why do you think filtering the output is better then
> > > not generating an useless output.
> > > It is useless because if I'm not debugging usb packets, I don't really
> > > need debug+ level for usbredir.
> > >
> > > Indeed, another env var is not nice. My main goal would be having
> > > fdo#91838 solved. But I don't want to touch that while spice-server is
> > > not yet GObjectify (to set proper domains).
> > >
> > > https://bugs.freedesktop.org/show_bug.cgi?id=91838
> > >
> > > But I really want to understand why filtering is okay for you. Doesn't
> > > it affect the runtime? printing all the time is not cheap I guess.
> >
> > At the rate spice-gtk prints, it is probably very very cheap, it is not
> > relevant to talk about performance here.
> >
> 
> usbredir prints a lot when we redirect a webcam...
> 
> > My point is greping (or other filtering tools) offers a lot more
> > flexibility to configure what you want to see.
> 
> Fair enough but I think this is more a personal opinion.
> 
> In my opinion, something like below to work with usbredir seems much
> better then filtering in/out the output:
> 
> SPICE_DEBUG=spice-channel:*,channel-usbredir:*
> 
> That seems doable even if only for spice-gtk. I'll try that later if
> that's seems reasonable.

As long as SPICE_DEBUG=1 keep working, and the patch is reasonable.

However, I think in the case of usbredir, it would make sense to attibute its own GLog domain, and not print usbredir messages in SPICE_DEBUG=1 log. This can be done in usbredir_log().
 
> 
> (without, of course, losing the generic SPICE_DEBUG=<int>)
> 
> Best,
> 
> >
> > > Cheers,
> > >   toso
> > > 
> > > >
> > > > >  
> > > > > > ---
> > > > > >  src/channel-usbredir.c | 12 +++++++++++-
> > > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > > > > > index 0be72ba..84cabe0 100644
> > > > > > --- a/src/channel-usbredir.c
> > > > > > +++ b/src/channel-usbredir.c
> > > > > > @@ -202,6 +202,16 @@ static void
> > > > > > channel_set_handlers(SpiceChannelClass
> > > > > > *klass)
> > > > > >  
> > > > > >  /*
> > > > > >  ------------------------------------------------------------------
> > > > > >  */
> > > > > >  /* private api
> > > > > >  */
> > > > > > +static gint
> > > > > > +usbredir_get_log_level(void)
> > > > > > +{
> > > > > > +    const gchar *debug_env = g_getenv("SPICE_DEBUG_USBREDIR");
> > > > > > +    if (debug_env != NULL) {
> > > > > > +        return CLAMP(atoi(debug_env), usbredirparser_none,
> > > > > > usbredirparser_debug_data);
> > > > > > +    }
> > > > > > +
> > > > > > +    return spice_util_get_debug() ? usbredirparser_debug :
> > > > > > usbredirparser_warning;
> > > > > > +}
> > > > > >  
> > > > > >  G_GNUC_INTERNAL
> > > > > >  void spice_usbredir_channel_set_context(SpiceUsbredirChannel
> > > > > >  *channel,
> > > > > > @@ -223,7 +233,7 @@ void
> > > > > > spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
> > > > > >                                     usbredir_unlock_lock,
> > > > > >                                     usbredir_free_lock,
> > > > > >                                     channel, PACKAGE_STRING,
> > > > > > -                                   spice_util_get_debug() ?
> > > > > > usbredirparser_debug : usbredirparser_warning,
> > > > > > +                                   usbredir_get_log_level(),
> > > > > >                                     usbredirhost_fl_write_cb_owns_buffer);
> > > > > >      if (!priv->host)
> > > > > >          g_error("Out of memory allocating usbredirhost");
> > > > > > --
> > > > > > 2.5.0
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Spice-devel mailing list
> > > > > > Spice-devel at lists.freedesktop.org
> > > > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > > > 
> > > > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>