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

[Christophe de Dinechin]

> 
> 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?

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.

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.


Christophe


> 
> #define SPICE_RESERVED_PADDING (10 * sizeof(void*))
> 
> -- 
> 2.9.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel