[Spice-devel] [PATCH spice-gtk] Remove unnecessary debug check from SPICE_DEBUG

[Jonathon Jongsma]

On Fri, 2017-06-09 at 11:33 +0200, Christophe de Dinechin wrote:
> > 
> > On 8 Jun 2017, at 23:17, Jonathon Jongsma <jjongsma at redhat.com>
> > wrote:
> > 
> > Calling spice_util_get_debug() from the SPICE_DEBUG() macro is
> > unnecessary since g_log() will already check whether the message
> > will
> > actually be printed. The only benefit to calling this function from
> > SPICE_DEBUG() is that it ensures that the SPICE_DEBUG environment
> > variable gets read the very first time we try to log something with
> > this macro. To solve this problem we instead use a constructor
> > function
> > to ensure that the env var is read at startup.
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> > ---
> > src/spice-util.c | 17 ++++++++++-------
> > src/spice-util.h |  7 ++-----
> > 2 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/spice-util.c b/src/spice-util.c
> > index 86377b6..848f20a 100644
> > --- a/src/spice-util.c
> > +++ b/src/spice-util.c
> > @@ -18,6 +18,7 @@
> > */
> > #include "config.h"
> > 
> > +#include <common/macros.h>
> > #include <stdbool.h>
> > #include <stdlib.h>
> > #include <string.h>
> > @@ -63,13 +64,6 @@ static void
> > spice_util_enable_debug_messages(void)
> >  **/
> > void spice_util_set_debug(gboolean enabled)
> > {
> > -    /* Make sure debug_once has been initialised
> > -     * with the value of SPICE_DEBUG already, otherwise
> > -     * spice_util_get_debug() may overwrite the value
> > -     * that was just set using spice_util_set_debug()
> > -     */
> > -    spice_util_get_debug();
> > -
> >     if (enabled) {
> >         spice_util_enable_debug_messages();
> >     }
> > @@ -88,6 +82,15 @@ static gpointer getenv_debug(gpointer data)
> >     return GINT_TO_POINTER(debug);
> > }
> > 
> > +/* Make sure debug_once has been initialised with the value of
> > SPICE_DEBUG at
> > + * startup, otherwise spice_util_get_debug() may overwrite the
> > value that is
> > + * set using spice_util_set_debug() */
> > +SPICE_CONSTRUCTOR_FUNC(spice_log_init)
> > +{
> > +    /* ensure that we enable debugging if the SPICE_DEBUG variable
> > is set */
> > +    spice_util_get_debug();
> > +}
> > +
> 
> This part is an improvement.
> 
> > gboolean spice_util_get_debug(void)
> > {
> >     g_once(&debug_once, getenv_debug, NULL);
> > diff --git a/src/spice-util.h b/src/spice-util.h
> > index a2a7683..7a95a9e 100644
> > --- a/src/spice-util.h
> > +++ b/src/spice-util.h
> > @@ -32,11 +32,8 @@ gulong spice_g_signal_connect_object(gpointer
> > instance,
> >                                      GConnectFlags connect_flags);
> > gchar* spice_uuid_to_string(const guint8 uuid[16]);
> > 
> > -#define SPICE_DEBUG(fmt, ...)                                   \
> > -    do {                                                        \
> > -        if (G_UNLIKELY(spice_util_get_debug()))                 \
> > -            g_debug(G_STRLOC " " fmt, ## __VA_ARGS__);          \
> > -    } while (0)
> > +#define SPICE_DEBUG(fmt, ...) \
> > +    g_debug(G_STRLOC " " fmt, ## __VA_ARGS__)
> 
> Why not connect SPICE_DEBUG to spice_debug instead of g_debug?

In the past, spice_debug() (which is defined in spice-common) was its
own logging implementation based roughly on the glib logging API. At
some point in the recent past, the spice_debug() implementation in
spice-common was switched to actually use the glib logging framework.
For historical reasons, however, spice-gtk generally doesn't use
spice_debug() and uses glib logging functions directly (via
SPICE_DEBUG()).


> I found it confusing enough that we had a conditional SPICE_DEBUG and
> a non-conditional spice_debug defined in different files. If now both
> are non-conditional, they might as well be merged.

 I agree that this situation needs to be cleared up. I was looking into
the spice-common logging implementation and I've found quite a few
significant problems that I'll be sending additional emails about soon.

> 
> FYI, you probably realize that, but this becomes unnecessary in my
> ‘traces’ branch, since a much cheaper mechanism is used to
> enable/disable logging on a per-category basis. So I’ve defined it as
> spice_trace(debug, …), but I’m actively hunting down the various
> XYZ_DEBUG statements.

Right, but there's no consensus on your proposal yet.

Jonathon