[Spice-devel] [spice-server 3/3] dcc: Add debug log when setting compression

[Christophe Fergeau]

Without this it's not obvious that a compression setting took effect.
---
server/dcc.c | 3 +++
server/utils.c | 42 ++++++++++++++++++++++++++++++++++++++++++
server/utils.h | 3 +++
3 files changed, 48 insertions(+)

diff --git a/server/dcc.c b/server/dcc.c
index 826dd28fe..e27d3dcad 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1093,6 +1093,9 @@ static bool dcc_handle_preferred_compression(DisplayChannelClient *dcc,
default:
spice_warning("preferred-compression: unsupported image compression setting");
}
+ g_debug("Setting preferred compression to %s",
+ spice_util_genum_get_nick(SPICE_TYPE_SPICE_IMAGE_COMPRESSION_T,
+ dcc->priv->image_compression));
return TRUE;
}

diff --git a/server/utils.c b/server/utils.c
index b8a40b1d9..4b53aa290 100644
--- a/server/utils.c
+++ b/server/utils.c
@@ -114,3 +114,45 @@ int red_channel_name_to_type(const char *name)
}
return -1;
}
+
+/* These 2 functions come from
+ * libvirt-glib/libvirt-gconfig/libvirt-gconfig-helpers.c
+ * Copyright (C) 2010, 2011 Red Hat, Inc.
+ * LGPLv2.1+ licensed */
+G_GNUC_INTERNAL const char *
+spice_util_genum_get_nick(GType enum_type, gint value)
+{
+ GEnumClass *enum_class;
+ GEnumValue *enum_value;
+
+ g_return_val_if_fail (G_TYPE_IS_ENUM (enum_type), NULL);
+
+ enum_class = g_type_class_ref(enum_type);
+ enum_value = g_enum_get_value(enum_class, value);
+ g_type_class_unref(enum_class);
+
+ if (enum_value != NULL)
+ return enum_value->value_nick;
+
+ g_return_val_if_reached(NULL);
+}
+
+G_GNUC_INTERNAL int
+spice_util_genum_get_value(GType enum_type, const char *nick,
+ gint default_value)
+{
+ GEnumClass *enum_class;
+ GEnumValue *enum_value;
+
+ g_return_val_if_fail(G_TYPE_IS_ENUM(enum_type), default_value);
+ g_return_val_if_fail(nick != NULL, default_value);
+
+ enum_class = g_type_class_ref(enum_type);
+ enum_value = g_enum_get_value_by_nick(enum_class, nick);
+ g_type_class_unref(enum_class);
+
+ if (enum_value != NULL)
+ return enum_value->value;
+
+ g_return_val_if_reached(default_value);
+}
diff --git a/server/utils.h b/server/utils.h
index 5cb0eb0ee..52b7f5b3e 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -21,6 +21,7 @@

#include <stdint.h>
#include <glib.h>
+#include <glib-object.h>

#define SPICE_GNUC_VISIBLE __attribute__ ((visibility ("default")))

@@ -55,5 +56,7 @@ int rgb32_data_has_alpha(int width, int height, size_t stride,

const char *red_channel_type_to_str(int type);
int red_channel_name_to_type(const char *name);
+int spice_util_genum_get_value(GType enum_type, const char *nick, gint default_value);
+const char *spice_util_genum_get_nick(GType enum_type, gint value);

#endif /* UTILS_H_ */

[Christophe Fergeau]

This is a thin wrapper over g_get_monotonic_time_ms, and is called only
once, so we can call directly g_get_monotonic_time_ms instead.
---
server/reds.c | 2 +-
server/utils.h | 5 -----
2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 4f890348a..4645f1c6b 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3004,7 +3004,7 @@ static void migrate_timeout(void *opaque)

uint32_t reds_get_mm_time(void)
{
- return spice_get_monotonic_time_ms();
+ return g_get_monotonic_time() / 1000;
}

void reds_enable_mm_time(RedsState *reds)
diff --git a/server/utils.h b/server/utils.h
index 36ad1d83f..5cb0eb0ee 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -50,11 +50,6 @@ static inline red_time_t spice_get_monotonic_time_ns(void)

#define MSEC_PER_SEC 1000

-static inline red_time_t spice_get_monotonic_time_ms(void)
-{
- return g_get_monotonic_time() / 1000;
-}
-
int rgb32_data_has_alpha(int width, int height, size_t stride,
const uint8_t *data, int *all_set_out);

[Frediano Ziglio]

Post by Christophe Fergeau
This is a thin wrapper over g_get_monotonic_time_ms, and is called only
once, so we can call directly g_get_monotonic_time_ms instead.

Acked-by: Frediano Ziglio <***@redhat.com>

Frediano

Post by Christophe Fergeau
---
server/reds.c | 2 +-
server/utils.h | 5 -----
2 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/server/reds.c b/server/reds.c
index 4f890348a..4645f1c6b 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3004,7 +3004,7 @@ static void migrate_timeout(void *opaque)
uint32_t reds_get_mm_time(void)
{
- return spice_get_monotonic_time_ms();
+ return g_get_monotonic_time() / 1000;
}
void reds_enable_mm_time(RedsState *reds)
diff --git a/server/utils.h b/server/utils.h
index 36ad1d83f..5cb0eb0ee 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -50,11 +50,6 @@ static inline red_time_t spice_get_monotonic_time_ns(void)
#define MSEC_PER_SEC 1000
-static inline red_time_t spice_get_monotonic_time_ms(void)
-{
- return g_get_monotonic_time() / 1000;
-}
-
int rgb32_data_has_alpha(int width, int height, size_t stride,
const uint8_t *data, int *all_set_out);

[Frediano Ziglio]

These function are used once each by red-qxl.c, they don't need to be
exported in utils.h
---
server/red-qxl.c | 17 +++++++++++++++++
server/utils.h | 16 ----------------
2 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/server/red-qxl.c b/server/red-qxl.c
index 97940611b..4f05abb4e 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -455,6 +455,17 @@ static void qxl_worker_reset_memslots(QXLWorker
*qxl_worker)
red_qxl_reset_memslots(qxl_state);
}
+static void set_bit(int index, uint32_t *addr)
+{
+ uint32_t mask = 1 << index;
+ __sync_or_and_fetch(addr, mask);
+}
+
+static int test_bit(int index, uint32_t val)
+{
+ return val & (1u << index);
+}
+
static bool red_qxl_set_pending(QXLState *qxl_state, int pending)
{
// this is not atomic but is not an issue
@@ -922,6 +933,12 @@ Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl)
return qxl->st->dispatcher;
}
+static void clear_bit(int index, uint32_t *addr)
+{
+ uint32_t mask = ~(1 << index);
+ __sync_and_and_fetch(addr, mask);
+}
+
void red_qxl_clear_pending(QXLState *qxl_state, int pending)
{
spice_return_if_fail(qxl_state != NULL);
diff --git a/server/utils.h b/server/utils.h
index 58d43cafe..36ad1d83f 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -24,22 +24,6 @@
#define SPICE_GNUC_VISIBLE __attribute__ ((visibility ("default")))
-static inline void set_bit(int index, uint32_t *addr)
-{
- uint32_t mask = 1 << index;
- __sync_or_and_fetch(addr, mask);
-}
-
-static inline void clear_bit(int index, uint32_t *addr)
-{
- uint32_t mask = ~(1 << index);
- __sync_and_and_fetch(addr, mask);
-}
-
-static inline int test_bit(int index, uint32_t val)
-{
- return val & (1u << index);
-}
/* a generic safe for loop macro */
#define SAFE_FOREACH(link, next, cond, ring, data, get_data) \
for ((((link) = ((cond) ? ring_get_head(ring) : NULL)), \

I prefer them in utils.h, they don't have something to do specifically
with qxl interface.

Frediano

[Christophe Fergeau]

Post by Frediano Ziglio

-static inline void set_bit(int index, uint32_t *addr)
-{
- uint32_t mask = 1 << index;
- __sync_or_and_fetch(addr, mask);
-}
-
-static inline void clear_bit(int index, uint32_t *addr)
-{
- uint32_t mask = ~(1 << index);
- __sync_and_and_fetch(addr, mask);
-}
-
-static inline int test_bit(int index, uint32_t val)
-{
- return val & (1u << index);
-}
/* a generic safe for loop macro */
#define SAFE_FOREACH(link, next, cond, ring, data, get_data) \
for ((((link) = ((cond) ? ring_get_head(ring) : NULL)), \

I prefer them in utils.h, they don't have something to do specifically
with qxl interface.

That's true, on the other hand, these are not methods we want to use
outside of the QXL interface.

Christophe

[Frediano Ziglio]

Post by Christophe Fergeau

Post by Frediano Ziglio

-static inline void set_bit(int index, uint32_t *addr)
-{
- uint32_t mask = 1 << index;
- __sync_or_and_fetch(addr, mask);
-}
-
-static inline void clear_bit(int index, uint32_t *addr)
-{
- uint32_t mask = ~(1 << index);
- __sync_and_and_fetch(addr, mask);
-}
-
-static inline int test_bit(int index, uint32_t val)
-{
- return val & (1u << index);
-}
/* a generic safe for loop macro */
#define SAFE_FOREACH(link, next, cond, ring, data, get_data)
\
for ((((link) = ((cond) ? ring_get_head(ring) : NULL)),
\

I prefer them in utils.h, they don't have something to do specifically
with qxl interface.

That's true, on the other hand, these are not methods we want to use
outside of the QXL interface.
Christophe

I prefer them in utils.h.

Frediano

[Frediano Ziglio]

Post by Christophe Fergeau
Without this it's not obvious that a compression setting took effect.
---
server/dcc.c | 3 +++
server/utils.c | 42 ++++++++++++++++++++++++++++++++++++++++++
server/utils.h | 3 +++
3 files changed, 48 insertions(+)
diff --git a/server/dcc.c b/server/dcc.c
index 826dd28fe..e27d3dcad 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1093,6 +1093,9 @@ static bool
dcc_handle_preferred_compression(DisplayChannelClient *dcc,
spice_warning("preferred-compression: unsupported image compression
setting");
}
+ g_debug("Setting preferred compression to %s",
+ spice_util_genum_get_nick(SPICE_TYPE_SPICE_IMAGE_COMPRESSION_T,
+ dcc->priv->image_compression));
return TRUE;
}
diff --git a/server/utils.c b/server/utils.c
index b8a40b1d9..4b53aa290 100644
--- a/server/utils.c
+++ b/server/utils.c
@@ -114,3 +114,45 @@ int red_channel_name_to_type(const char *name)
}
return -1;
}
+
+/* These 2 functions come from
+ * libvirt-glib/libvirt-gconfig/libvirt-gconfig-helpers.c
+ * Copyright (C) 2010, 2011 Red Hat, Inc.
+ * LGPLv2.1+ licensed */
+G_GNUC_INTERNAL const char *
+spice_util_genum_get_nick(GType enum_type, gint value)
+{
+ GEnumClass *enum_class;
+ GEnumValue *enum_value;
+
+ g_return_val_if_fail (G_TYPE_IS_ENUM (enum_type), NULL);
+
+ enum_class = g_type_class_ref(enum_type);
+ enum_value = g_enum_get_value(enum_class, value);
+ g_type_class_unref(enum_class);
+
+ if (enum_value != NULL)
+ return enum_value->value_nick;
+
+ g_return_val_if_reached(NULL);
+}
+
+G_GNUC_INTERNAL int
+spice_util_genum_get_value(GType enum_type, const char *nick,
+ gint default_value)
+{
+ GEnumClass *enum_class;
+ GEnumValue *enum_value;
+
+ g_return_val_if_fail(G_TYPE_IS_ENUM(enum_type), default_value);
+ g_return_val_if_fail(nick != NULL, default_value);
+
+ enum_class = g_type_class_ref(enum_type);
+ enum_value = g_enum_get_value_by_nick(enum_class, nick);
+ g_type_class_unref(enum_class);
+
+ if (enum_value != NULL)
+ return enum_value->value;
+
+ g_return_val_if_reached(default_value);
+}
diff --git a/server/utils.h b/server/utils.h
index 5cb0eb0ee..52b7f5b3e 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -21,6 +21,7 @@
#include <stdint.h>
#include <glib.h>
+#include <glib-object.h>
#define SPICE_GNUC_VISIBLE __attribute__ ((visibility ("default")))
@@ -55,5 +56,7 @@ int rgb32_data_has_alpha(int width, int height, size_t stride,
const char *red_channel_type_to_str(int type);
int red_channel_name_to_type(const char *name);
+int spice_util_genum_get_value(GType enum_type, const char *nick, gint default_value);
+const char *spice_util_genum_get_nick(GType enum_type, gint value);
#endif /* UTILS_H_ */

If you follow the same role of 1/3 these new functions should be
in dcc.c.
I don't understand why you need to have this in the log.
Are you writing a test?
Would %d format not be enough? Would save 46 lines. Also
would support cases where the enumeration is not in the list.

Frediano

[Christophe Fergeau]

Post by Frediano Ziglio

Post by Christophe Fergeau
diff --git a/server/utils.h b/server/utils.h
index 5cb0eb0ee..52b7f5b3e 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -21,6 +21,7 @@
#include <stdint.h>
#include <glib.h>
+#include <glib-object.h>
#define SPICE_GNUC_VISIBLE __attribute__ ((visibility ("default")))
@@ -55,5 +56,7 @@ int rgb32_data_has_alpha(int width, int height, size_t stride,
const char *red_channel_type_to_str(int type);
int red_channel_name_to_type(const char *name);
+int spice_util_genum_get_value(GType enum_type, const char *nick, gint default_value);
+const char *spice_util_genum_get_nick(GType enum_type, gint value);
#endif /* UTILS_H_ */

If you follow the same role of 1/3 these new functions should be
in dcc.c.

Haha that's a fair point :) In this case, this is a (generic) helper I
hope we'll use in more places.

Post by Frediano Ziglio
I don't understand why you need to have this in the log.
Are you writing a test?

It's related to https://bugzilla.redhat.com/show_bug.cgi?id=1460191
where it was not clear which type of compression was being used when the
client is used with --spice-preferred-compression

Post by Frediano Ziglio
Would %d format not be enough? Would save 46 lines. Also
would support cases where the enumeration is not in the list.

From that bug (but client side):
(virt-viewer:10238): GSpice-DEBUG: channel-display.c:526 display-2:0:
changing preferred compression to 7

Isn't it better to show 'lz4' rather than 7? :) A similar thing was done
for 'display' actually, it shows the channel name rather than just '2'
(or whatever int value it has).

So yes, this adds more lines, but imo it's worth it, it's a one time
cost to get human readable enum names in more places.

Christophe

Post by Frediano Ziglio
Frediano

[Frediano Ziglio]

Post by Christophe Fergeau
Without this it's not obvious that a compression setting took effect.

We spoke about, there was also a request for this debug line.

Post by Christophe Fergeau
---
server/dcc.c | 3 +++
server/utils.c | 42 ++++++++++++++++++++++++++++++++++++++++++
server/utils.h | 3 +++
3 files changed, 48 insertions(+)
diff --git a/server/dcc.c b/server/dcc.c
index 826dd28fe..e27d3dcad 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -1093,6 +1093,9 @@ static bool
dcc_handle_preferred_compression(DisplayChannelClient *dcc,
spice_warning("preferred-compression: unsupported image compression
setting");
}
+ g_debug("Setting preferred compression to %s",
+ spice_util_genum_get_nick(SPICE_TYPE_SPICE_IMAGE_COMPRESSION_T,
+ dcc->priv->image_compression));
return TRUE;
}
diff --git a/server/utils.c b/server/utils.c
index b8a40b1d9..4b53aa290 100644
--- a/server/utils.c
+++ b/server/utils.c
@@ -114,3 +114,45 @@ int red_channel_name_to_type(const char *name)
}
return -1;
}
+
+/* These 2 functions come from
+ * libvirt-glib/libvirt-gconfig/libvirt-gconfig-helpers.c
+ * Copyright (C) 2010, 2011 Red Hat, Inc.
+ * LGPLv2.1+ licensed */

the license is the same, no reason to state here, if it was different
I would prefer to put these in another new file.

Post by Christophe Fergeau
+G_GNUC_INTERNAL const char *

We don't use G_GNUC_INTERNAL in spice-server and spice-common, not
that hurts but is more coherent without.

OT: maybe these would be also useful in spice-common? I remember
Victor once try to write something like this for spice-gtk.

Post by Christophe Fergeau
+spice_util_genum_get_nick(GType enum_type, gint value)

I don't like much the name:
- in spice-server we tend to use spice_ prefix for public function
(with some old function name exception);
- the function is not dependent to spice, just gobject;
- don't like much the _util_ in the name.

Maybe just genum_get_nick? Or just use the classic prefix, being
red_genum_get_nick.

Post by Christophe Fergeau
+{
+ GEnumClass *enum_class;
+ GEnumValue *enum_value;
+
+ g_return_val_if_fail (G_TYPE_IS_ENUM (enum_type), NULL);

spaces style

Post by Christophe Fergeau
+
+ enum_class = g_type_class_ref(enum_type);
+ enum_value = g_enum_get_value(enum_class, value);
+ g_type_class_unref(enum_class);
+
+ if (enum_value != NULL)
+ return enum_value->value_nick;

missing brackets

Post by Christophe Fergeau
+
+ g_return_val_if_reached(NULL);
+}
+
+G_GNUC_INTERNAL int
+spice_util_genum_get_value(GType enum_type, const char *nick,

similar comments for this function

Post by Christophe Fergeau
+ gint default_value)
+{
+ GEnumClass *enum_class;
+ GEnumValue *enum_value;
+
+ g_return_val_if_fail(G_TYPE_IS_ENUM(enum_type), default_value);
+ g_return_val_if_fail(nick != NULL, default_value);
+
+ enum_class = g_type_class_ref(enum_type);
+ enum_value = g_enum_get_value_by_nick(enum_class, nick);
+ g_type_class_unref(enum_class);
+
+ if (enum_value != NULL)
+ return enum_value->value;

missing brackets

Post by Christophe Fergeau
+
+ g_return_val_if_reached(default_value);
+}
diff --git a/server/utils.h b/server/utils.h
index 5cb0eb0ee..52b7f5b3e 100644
--- a/server/utils.h
+++ b/server/utils.h
@@ -21,6 +21,7 @@
#include <stdint.h>
#include <glib.h>
+#include <glib-object.h>
#define SPICE_GNUC_VISIBLE __attribute__ ((visibility ("default")))
@@ -55,5 +56,7 @@ int rgb32_data_has_alpha(int width, int height, size_t stride,
const char *red_channel_type_to_str(int type);
int red_channel_name_to_type(const char *name);
+int spice_util_genum_get_value(GType enum_type, const char *nick, gint default_value);
+const char *spice_util_genum_get_nick(GType enum_type, gint value);
#endif /* UTILS_H_ */

Frediano

[Christophe Fergeau]

Post by Frediano Ziglio

Post by Christophe Fergeau
+
+/* These 2 functions come from
+ * libvirt-glib/libvirt-gconfig/libvirt-gconfig-helpers.c
+ * Copyright (C) 2010, 2011 Red Hat, Inc.
+ * LGPLv2.1+ licensed */

the license is the same, no reason to state here, if it was different
I would prefer to put these in another new file.

I want to keep the reference to the source file it's copied from so that
we know where the code's coming from, once we have this, imo it does not
hurt to have an indication that everything is fine licence-wise. So I'd
prefer to keep it.

Post by Frediano Ziglio

Post by Christophe Fergeau
+G_GNUC_INTERNAL const char *

We don't use G_GNUC_INTERNAL in spice-server and spice-common, not
that hurts but is more coherent without.

Yup removed.

Post by Frediano Ziglio
OT: maybe these would be also useful in spice-common? I remember
Victor once try to write something like this for spice-gtk.

That's an option, this means keeping the spice_ namespacing,
spice-common does not use red_. I'll send a patch moving this to
spice-common.

I've fixed the style issues that you mentioned.

Christophe