[spice-gtk,v1,1/3] Improve debug log for preferred compression message

Submitted by Victor Toso on Dec. 20, 2017, 1:18 p.m.

Details

Message ID 20171220131803.22281-1-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 Dec. 20, 2017, 1:18 p.m.
From: Victor Toso <me@victortoso.com>

To use a string instead of number (enum)

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/channel-display.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display.c b/src/channel-display.c
index 75d2e32..dece3b9 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -536,6 +536,29 @@  void spice_display_change_preferred_compression(SpiceChannel *channel, gint comp
     spice_display_channel_change_preferred_compression(channel, compression);
 }
 
+static const gchar *image_compression_types_str[] = {
+    [ SPICE_IMAGE_COMPRESSION_INVALID ] = "invalid",
+    [ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
+    [ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto-glz",
+    [ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto-lz",
+    [ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
+    [ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
+    [ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
+    [ SPICE_IMAGE_COMPRESSION_LZ4 ] = "lz4",
+};
+G_STATIC_ASSERT(G_N_ELEMENTS(image_compression_types_str) <= SPICE_IMAGE_COMPRESSION_ENUM_END);
+
+static const gchar *preferred_compression_type_to_string(gint type)
+{
+    const char *str = NULL;
+
+    if (type >= 0 && type < G_N_ELEMENTS(image_compression_types_str)) {
+        str = image_compression_types_str[type];
+    }
+
+    return str ? str : "unknown";
+}
+
 /**
  * spice_display_channel_change_preferred_compression:
  * @channel: a #SpiceDisplayChannel
@@ -560,7 +583,8 @@  void spice_display_channel_change_preferred_compression(SpiceChannel *channel, g
         return;
     }
 
-    CHANNEL_DEBUG(channel, "changing preferred compression to %d", compression);
+    CHANNEL_DEBUG(channel, "changing preferred compression to %s",
+                  preferred_compression_type_to_string(compression));
 
     pref_comp_msg.image_compression = compression;
     out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION);

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> To use a string instead of number (enum)
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/channel-display.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 75d2e32..dece3b9 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -536,6 +536,29 @@ void
> spice_display_change_preferred_compression(SpiceChannel *channel, gint comp
>      spice_display_channel_change_preferred_compression(channel,
>      compression);
>  }
>  
> +static const gchar *image_compression_types_str[] = {

+const

> +    [ SPICE_IMAGE_COMPRESSION_INVALID ] = "invalid",
> +    [ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
> +    [ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto-glz",
> +    [ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto-lz",
> +    [ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
> +    [ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
> +    [ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
> +    [ SPICE_IMAGE_COMPRESSION_LZ4 ] = "lz4",
> +};
> +G_STATIC_ASSERT(G_N_ELEMENTS(image_compression_types_str) <=
> SPICE_IMAGE_COMPRESSION_ENUM_END);
> +
> +static const gchar *preferred_compression_type_to_string(gint type)
> +{
> +    const char *str = NULL;
> +
> +    if (type >= 0 && type < G_N_ELEMENTS(image_compression_types_str)) {
> +        str = image_compression_types_str[type];
> +    }
> +
> +    return str ? str : "unknown";
> +}
> +
>  /**
>   * spice_display_channel_change_preferred_compression:
>   * @channel: a #SpiceDisplayChannel
> @@ -560,7 +583,8 @@ void
> spice_display_channel_change_preferred_compression(SpiceChannel *channel, g
>          return;
>      }
>  
> -    CHANNEL_DEBUG(channel, "changing preferred compression to %d",
> compression);
> +    CHANNEL_DEBUG(channel, "changing preferred compression to %s",
> +                  preferred_compression_type_to_string(compression));
>  
>      pref_comp_msg.image_compression = compression;
>      out = spice_msg_out_new(channel,
>      SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION);

There's a similar array in spice-session.c (_spice_image_compress_values) and
similar values in spice-option.c (parse_preferred_compression).

To avoid such duplications I usually use a macro like

#define SPICE_ALL_COMPRESSIONS \
    SPICE_COMPRESSION(SPICE_IMAGE_COMPRESSION_OFF, "off) \
    SPICE_COMPRESSION(SPICE_IMAGE_COMPRESSION_INVALID, "invalid") \
...

and use like

static const GEnumValue _spice_image_compress_values[] = {
#define SPICE_COMPRESSION(name, str) { name, #name, str },
    SPICE_ALL_COMPRESSIONS
#undef SPICE_COMPRESSION
  { 0, NULL, NULL }
};

or 

static const gchar *const image_compression_types_str[] = {
#define SPICE_COMPRESSION(name, str) [ name ] = str,
    SPICE_ALL_COMPRESSIONS
#undef SPICE_COMPRESSION
};

In this case you could make _spice_image_compress_values public and
use it. Yes, maybe you don't want to slow down the debug code (surely
not a problem for option parsing).

Frediano
On 12/20/2017 03:18 PM, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>

Hi Victor,

> 
> To use a string instead of number (enum)
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>   src/channel-display.c | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 75d2e32..dece3b9 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -536,6 +536,29 @@ void spice_display_change_preferred_compression(SpiceChannel *channel, gint comp
>       spice_display_channel_change_preferred_compression(channel, compression);
>   }
>   
> +static const gchar *image_compression_types_str[] = {
> +    [ SPICE_IMAGE_COMPRESSION_INVALID ] = "invalid",
> +    [ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
> +    [ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto-glz",
> +    [ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto-lz",
> +    [ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
> +    [ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
> +    [ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
> +    [ SPICE_IMAGE_COMPRESSION_LZ4 ] = "lz4",
> +};
> +G_STATIC_ASSERT(G_N_ELEMENTS(image_compression_types_str) <= SPICE_IMAGE_COMPRESSION_ENUM_END);
> +
> +static const gchar *preferred_compression_type_to_string(gint type)
> +{
> +    const char *str = NULL;

It would be simpler to initialize str = "unknown";
> +
> +    if (type >= 0 && type < G_N_ELEMENTS(image_compression_types_str)) {
> +        str = image_compression_types_str[type];
> +    }
> +
> +    return str ? str : "unknown";

And simply return str; here

Uri.

> +}
> +
>   /**
>    * spice_display_channel_change_preferred_compression:
>    * @channel: a #SpiceDisplayChannel
> @@ -560,7 +583,8 @@ void spice_display_channel_change_preferred_compression(SpiceChannel *channel, g
>           return;
>       }
>   
> -    CHANNEL_DEBUG(channel, "changing preferred compression to %d", compression);
> +    CHANNEL_DEBUG(channel, "changing preferred compression to %s",
> +                  preferred_compression_type_to_string(compression));
>   
>       pref_comp_msg.image_compression = compression;
>       out = spice_msg_out_new(channel, SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION);
>
> 
> On 12/20/2017 03:18 PM, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> 
> Hi Victor,
> 
> > 
> > To use a string instead of number (enum)
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >   src/channel-display.c | 26 +++++++++++++++++++++++++-
> >   1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index 75d2e32..dece3b9 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -536,6 +536,29 @@ void
> > spice_display_change_preferred_compression(SpiceChannel *channel, gint
> > comp
> >       spice_display_channel_change_preferred_compression(channel,
> >       compression);
> >   }
> >   
> > +static const gchar *image_compression_types_str[] = {
> > +    [ SPICE_IMAGE_COMPRESSION_INVALID ] = "invalid",
> > +    [ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
> > +    [ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto-glz",
> > +    [ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto-lz",
> > +    [ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
> > +    [ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
> > +    [ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
> > +    [ SPICE_IMAGE_COMPRESSION_LZ4 ] = "lz4",
> > +};
> > +G_STATIC_ASSERT(G_N_ELEMENTS(image_compression_types_str) <=
> > SPICE_IMAGE_COMPRESSION_ENUM_END);
> > +
> > +static const gchar *preferred_compression_type_to_string(gint type)
> > +{
> > +    const char *str = NULL;
> 
> It would be simpler to initialize str = "unknown";
> > +
> > +    if (type >= 0 && type < G_N_ELEMENTS(image_compression_types_str)) {
> > +        str = image_compression_types_str[type];
> > +    }
> > +
> > +    return str ? str : "unknown";
> 
> And simply return str; here
> 
> Uri.
> 

Does not work if there are holes in the array.

Maybe we should define an helper for this and have

const gchar *enum_value_to_string(gint value, const gchar *const *values, size_t num_values)
{
    const char *str = NULL;

    if (value >= 0 && value < num_values) {
        str = values[value];
    }

    return str ? str : "unknown";
}

static const gchar *preferred_compression_type_to_string(gint type)
{
   return enum_value_to_string(type, image_compression_types_str, G_N_ELEMENTS(image_compression_types_str));
}

??

Frediano

> > +}
> > +
> >   /**
> >    * spice_display_channel_change_preferred_compression:
> >    * @channel: a #SpiceDisplayChannel
> > @@ -560,7 +583,8 @@ void
> > spice_display_channel_change_preferred_compression(SpiceChannel *channel,
> > g
> >           return;
> >       }
> >   
> > -    CHANNEL_DEBUG(channel, "changing preferred compression to %d",
> > compression);
> > +    CHANNEL_DEBUG(channel, "changing preferred compression to %s",
> > +                  preferred_compression_type_to_string(compression));
> >   
> >       pref_comp_msg.image_compression = compression;
> >       out = spice_msg_out_new(channel,
> >       SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION);
> > 
>
Hi,

On Wed, Dec 20, 2017 at 09:41:30AM -0500, Frediano Ziglio wrote:
> > 
> > On 12/20/2017 03:18 PM, Victor Toso wrote:
> > > From: Victor Toso <me@victortoso.com>
> > 
> > Hi Victor,
> > 
> > > 
> > > To use a string instead of number (enum)
> > > 
> > > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > > ---
> > >   src/channel-display.c | 26 +++++++++++++++++++++++++-
> > >   1 file changed, 25 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/channel-display.c b/src/channel-display.c
> > > index 75d2e32..dece3b9 100644
> > > --- a/src/channel-display.c
> > > +++ b/src/channel-display.c
> > > @@ -536,6 +536,29 @@ void
> > > spice_display_change_preferred_compression(SpiceChannel *channel, gint
> > > comp
> > >       spice_display_channel_change_preferred_compression(channel,
> > >       compression);
> > >   }
> > >   
> > > +static const gchar *image_compression_types_str[] = {
> > > +    [ SPICE_IMAGE_COMPRESSION_INVALID ] = "invalid",
> > > +    [ SPICE_IMAGE_COMPRESSION_OFF ] = "off",
> > > +    [ SPICE_IMAGE_COMPRESSION_AUTO_GLZ ] = "auto-glz",
> > > +    [ SPICE_IMAGE_COMPRESSION_AUTO_LZ ] = "auto-lz",
> > > +    [ SPICE_IMAGE_COMPRESSION_QUIC ] = "quic",
> > > +    [ SPICE_IMAGE_COMPRESSION_GLZ ] = "glz",
> > > +    [ SPICE_IMAGE_COMPRESSION_LZ ] = "lz",
> > > +    [ SPICE_IMAGE_COMPRESSION_LZ4 ] = "lz4",
> > > +};
> > > +G_STATIC_ASSERT(G_N_ELEMENTS(image_compression_types_str) <=
> > > SPICE_IMAGE_COMPRESSION_ENUM_END);
> > > +
> > > +static const gchar *preferred_compression_type_to_string(gint type)
> > > +{
> > > +    const char *str = NULL;
> > 
> > It would be simpler to initialize str = "unknown";
> > > +
> > > +    if (type >= 0 && type < G_N_ELEMENTS(image_compression_types_str)) {
> > > +        str = image_compression_types_str[type];
> > > +    }
> > > +
> > > +    return str ? str : "unknown";
> > 
> > And simply return str; here
> > 
> > Uri.
> > 
> 
> Does not work if there are holes in the array.
> 
> Maybe we should define an helper for this and have
> 
> const gchar *enum_value_to_string(gint value, const gchar *const *values, size_t num_values)
> {
>     const char *str = NULL;
> 
>     if (value >= 0 && value < num_values) {
>         str = values[value];
>     }
> 
>     return str ? str : "unknown";
> }
> 
> static const gchar *preferred_compression_type_to_string(gint type)
> {
>    return enum_value_to_string(type, image_compression_types_str, G_N_ELEMENTS(image_compression_types_str));
> }
> 
> ??

To be honest, I wrote this patch 3 times because of this. There
are other places in the source code that we do enum-to-string
conversion and I was thinking in making this better now too.

But then I thought that I was complicating things :)

I was thinking in a similar approach as you wrote above but if we
do that, we should make it in spice-common to benefit spice
server too, agree?

Cheers,
        toso

> 
> Frediano
> 
> > > +}
> > > +
> > >   /**
> > >    * spice_display_channel_change_preferred_compression:
> > >    * @channel: a #SpiceDisplayChannel
> > > @@ -560,7 +583,8 @@ void
> > > spice_display_channel_change_preferred_compression(SpiceChannel *channel,
> > > g
> > >           return;
> > >       }
> > >   
> > > -    CHANNEL_DEBUG(channel, "changing preferred compression to %d",
> > > compression);
> > > +    CHANNEL_DEBUG(channel, "changing preferred compression to %s",
> > > +                  preferred_compression_type_to_string(compression));
> > >   
> > >       pref_comp_msg.image_compression = compression;
> > >       out = spice_msg_out_new(channel,
> > >       SPICE_MSGC_DISPLAY_PREFERRED_COMPRESSION);
> > > 
> > 
>
On Wed, Dec 20, 2017 at 03:47:25PM +0100, Victor Toso wrote:
> To be honest, I wrote this patch 3 times because of this. There
> are other places in the source code that we do enum-to-string
> conversion and I was thinking in making this better now too.
> 
> But then I thought that I was complicating things :)
> 
> I was thinking in a similar approach as you wrote above but if we
> do that, we should make it in spice-common to benefit spice
> server too, agree?

For this specific case, spice-server already has some hack to feed the
image compression enum through glib-mkenums
https://cgit.freedesktop.org/spice/spice/tree/server/spice-server.h#n74

Once you have this, you can already use some generic code to convert
enum value to a string:
https://git.gnome.org/browse/libgovirt/tree/govirt/ovirt-utils.c#n230