[glib/wip/gvariant-kdbus: 14/17] GVariant: clean up serialised data handling API

[Ryan Lortie <desrt src gnome org>]

commit 127fc045925095685df015d570669e010254acd9
Author: Ryan Lortie <desrt desrt ca>
Date:   Sat Nov 29 15:58:49 2014 -0500

    GVariant: clean up serialised data handling API
    
    Add a new pair of internal helpers to gvariant-core.c to provide a
    unified way to construct and query the content of serialised GVariant
    instances.
    
    Rewrite g_variant_new_from_data() to use this.
    
    Move g_variant_new_from_bytes() and g_variant_get_data_as_bytes() out of
    -core and into gvariant.c, also rewriting them to use the same.
    
    Take the time to do some cleanup and make some general improvements in
    consistency:
    
     - move the checks for improperly sized fixed-sized data out of
       _new_from_bytes() and into the common code so that we do this check
       on _new_from_data() as well
    
     - correctly deal with the case of NULL data in _get_data_as_bytes().
       This would have crashed before.  Add a test for that.
    
     - if the user hands us data with a size of zero then unref and/or
       destroy-notify things immediately.
    
    The idea that every GVariant must have an associated GBytes remains.
    This could potentially be optimsed a bit further in the future, but the
    cases where it could be avoided are only a few (errors, zero-size,
    static stoarge) so let's not pursue that now.

 glib/gvariant-core.c  |  169 ++++++++++++++++++++++++-------------------------
 glib/gvariant-core.h  |   10 +++
 glib/gvariant.c       |   95 ++++++++++++++++++++++++---
 glib/tests/gvariant.c |   38 +++++++++++
 4 files changed, 215 insertions(+), 97 deletions(-)
---
diff --git a/glib/gvariant-core.c b/glib/gvariant-core.c
index 0f68168..1a0c041 100644
--- a/glib/gvariant-core.c
+++ b/glib/gvariant-core.c
@@ -509,58 +509,6 @@ g_variant_alloc (const GVariantType *type,
   return value;
 }
 
-/**
- * g_variant_new_from_bytes:
- * @type: a #GVariantType
- * @bytes: a #GBytes
- * @trusted: if the contents of @bytes are trusted
- *
- * Constructs a new serialised-mode #GVariant instance.  This is the
- * inner interface for creation of new serialised values that gets
- * called from various functions in gvariant.c.
- *
- * A reference is taken on @bytes.
- *
- * Returns: (transfer none): a new #GVariant with a floating reference
- *
- * Since: 2.36
- */
-GVariant *
-g_variant_new_from_bytes (const GVariantType *type,
-                          GBytes             *bytes,
-                          gboolean            trusted)
-{
-  GVariant *value;
-  guint alignment;
-  gsize size;
-
-  value = g_variant_alloc (type, TRUE, trusted);
-
-  value->contents.serialised.bytes = g_bytes_ref (bytes);
-
-  g_variant_type_info_query (value->type_info,
-                             &alignment, &size);
-
-  if (size && g_bytes_get_size (bytes) != size)
-    {
-      /* Creating a fixed-sized GVariant with a bytes of the wrong
-       * size.
-       *
-       * We should do the equivalent of pulling a fixed-sized child out
-       * of a brozen container (ie: data is NULL size is equal to the correct
-       * fixed size).
-       */
-      value->contents.serialised.data = NULL;
-      value->size = size;
-    }
-  else
-    {
-      value->contents.serialised.data = g_bytes_get_data (bytes, &value->size);
-    }
-
-  return value;
-}
-
 /* -- internal -- */
 
 /* < internal >
@@ -597,6 +545,63 @@ g_variant_new_from_children (const GVariantType  *type,
 }
 
 /* < internal >
+ * g_variant_new_serialised:
+ * @type: a #GVariantType
+ * @bytes: the #GBytes holding @data
+ * @data: a pointer to the serialised data
+ * @size: the size of @data, in bytes
+ * @trusted: %TRUE if @data is trusted
+ *
+ * Constructs a new serialised #GVariant instance.  This is the inner
+ * interface for creation of new serialised values that gets called from
+ * various functions in gvariant.c.
+ *
+ * @bytes is consumed by this function.  g_bytes_unref() will be called
+ * on it some time later.
+ *
+ * Returns: a new #GVariant with a floating reference
+ */
+GVariant *
+g_variant_new_serialised (const GVariantType *type,
+                          GBytes             *bytes,
+                          gconstpointer       data,
+                          gsize               size,
+                          gboolean            trusted)
+{
+  GVariant *value;
+  gsize fixed_size;
+
+  value = g_variant_alloc (type, TRUE, trusted);
+  value->contents.serialised.bytes = bytes;
+  value->contents.serialised.data = data;
+  value->size = size;
+
+  g_variant_type_info_query (value->type_info, NULL, &fixed_size);
+  if G_UNLIKELY (fixed_size && size != fixed_size)
+    {
+      /* Creating a fixed-sized GVariant with a bytes of the wrong
+       * size.
+       *
+       * We should do the equivalent of pulling a fixed-sized child out
+       * of a broken container (ie: data is NULL size is equal to the correct
+       * fixed size).
+       *
+       * This really ought not to happen if the data is trusted...
+       */
+      if (trusted)
+        g_error ("Attempting to create a trusted GVariant instance out of invalid data");
+
+      /* We hang on to the GBytes (even though we don't use it anymore)
+       * because every GVariant must have a GBytes.
+       */
+      value->contents.serialised.data = NULL;
+      value->size = fixed_size;
+    }
+
+  return value;
+}
+
+/* < internal >
  * g_variant_get_type_info:
  * @value: a #GVariant
  *
@@ -634,6 +639,32 @@ g_variant_is_trusted (GVariant *value)
   return (value->state & STATE_TRUSTED) != 0;
 }
 
+/* < internal >
+ * g_variant_get_serialised:
+ * @value: a #GVariant
+ * @bytes: (out) (transfer none): a location to store the #GBytes
+ * @size: (out): a location to store the size of the returned data
+ *
+ * Ensures that @value is in serialised form and returns information
+ * about it.  This is called from various APIs in gvariant.c
+ *
+ * Returns: data, of length @size
+ */
+gconstpointer
+g_variant_get_serialised (GVariant  *value,
+                          GBytes   **bytes,
+                          gsize     *size)
+{
+  g_variant_ensure_serialised (value);
+
+  if (bytes)
+    *bytes = value->contents.serialised.bytes;
+
+  *size = value->size;
+
+  return value->contents.serialised.data;
+}
+
 /* -- public -- */
 
 /**
@@ -885,40 +916,6 @@ g_variant_get_data (GVariant *value)
   return value->contents.serialised.data;
 }
 
-/**
- * g_variant_get_data_as_bytes:
- * @value: a #GVariant
- *
- * Returns a pointer to the serialised form of a #GVariant instance.
- * The semantics of this function are exactly the same as
- * g_variant_get_data(), except that the returned #GBytes holds
- * a reference to the variant data.
- *
- * Returns: (transfer full): A new #GBytes representing the variant data
- *
- * Since: 2.36
- */ 
-GBytes *
-g_variant_get_data_as_bytes (GVariant *value)
-{
-  const gchar *bytes_data;
-  const gchar *data;
-  gsize bytes_size;
-  gsize size;
-
-  g_variant_ensure_serialised (value);
-
-  bytes_data = g_bytes_get_data (value->contents.serialised.bytes, &bytes_size);
-  data = value->contents.serialised.data;
-  size = value->size;
-
-  if (data == bytes_data && size == bytes_size)
-    return g_bytes_ref (value->contents.serialised.bytes);
-  else
-    return g_bytes_new_from_bytes (value->contents.serialised.bytes,
-                                   data - bytes_data, size);
-}
-
 
 /**
  * g_variant_n_children:
diff --git a/glib/gvariant-core.h b/glib/gvariant-core.h
index 034dd43..8e7d6cc 100644
--- a/glib/gvariant-core.h
+++ b/glib/gvariant-core.h
@@ -30,8 +30,18 @@ GVariant *              g_variant_new_from_children                     (const G
                                                                          gsize                n_children,
                                                                          gboolean             trusted);
 
+GVariant *              g_variant_new_serialised                        (const GVariantType  *type,
+                                                                         GBytes              *bytes,
+                                                                         gconstpointer        data,
+                                                                         gsize                size,
+                                                                         gboolean             trusted);
+
 gboolean                g_variant_is_trusted                            (GVariant            *value);
 
 GVariantTypeInfo *      g_variant_get_type_info                         (GVariant            *value);
 
+gconstpointer           g_variant_get_serialised                        (GVariant            *value,
+                                                                         GBytes             **bytes,
+                                                                         gsize               *size);
+
 #endif /* __G_VARIANT_CORE_H__ */
diff --git a/glib/gvariant.c b/glib/gvariant.c
index 08421bf..7251d33 100644
--- a/glib/gvariant.c
+++ b/glib/gvariant.c
@@ -288,14 +288,9 @@ g_variant_new_from_trusted (const GVariantType *type,
                             gconstpointer       data,
                             gsize               size)
 {
-  GVariant *value;
-  GBytes *bytes;
-
-  bytes = g_bytes_new (data, size);
-  value = g_variant_new_from_bytes (type, bytes, TRUE);
-  g_bytes_unref (bytes);
+  gpointer mydata = g_memdup (data, size);
 
-  return value;
+  return g_variant_new_serialised (type, g_bytes_new_take (mydata, size), mydata, size, TRUE);
 }
 
 /**
@@ -5932,21 +5927,99 @@ g_variant_new_from_data (const GVariantType *type,
                          GDestroyNotify      notify,
                          gpointer            user_data)
 {
-  GVariant *value;
   GBytes *bytes;
 
   g_return_val_if_fail (g_variant_type_is_definite (type), NULL);
   g_return_val_if_fail (data != NULL || size == 0, NULL);
 
+  if (size == 0)
+    {
+      if (notify)
+        {
+          (* notify) (user_data);
+          notify = NULL;
+        }
+
+      data = NULL;
+    }
+
   if (notify)
     bytes = g_bytes_new_with_free_func (data, size, notify, user_data);
   else
     bytes = g_bytes_new_static (data, size);
 
-  value = g_variant_new_from_bytes (type, bytes, trusted);
-  g_bytes_unref (bytes);
+  return g_variant_new_serialised (type, bytes, data, size, trusted);
+}
 
-  return value;
+/**
+ * g_variant_new_from_bytes:
+ * @type: a #GVariantType
+ * @bytes: a #GBytes
+ * @trusted: if the contents of @bytes are trusted
+ *
+ * Constructs a new serialised-mode #GVariant instance.  This is the
+ * inner interface for creation of new serialised values that gets
+ * called from various functions in gvariant.c.
+ *
+ * A reference is taken on @bytes.
+ *
+ * Returns: (transfer none): a new #GVariant with a floating reference
+ *
+ * Since: 2.36
+ */
+GVariant *
+g_variant_new_from_bytes (const GVariantType *type,
+                          GBytes             *bytes,
+                          gboolean            trusted)
+{
+  gconstpointer data;
+  gsize size;
+
+  g_return_val_if_fail (g_variant_type_is_definite (type), NULL);
+
+  data = g_bytes_get_data (bytes, &size);
+
+  return g_variant_new_serialised (type, g_bytes_ref (bytes), data, size, trusted);
+}
+
+/**
+ * g_variant_get_data_as_bytes:
+ * @value: a #GVariant
+ *
+ * Returns a pointer to the serialised form of a #GVariant instance.
+ * The semantics of this function are exactly the same as
+ * g_variant_get_data(), except that the returned #GBytes holds
+ * a reference to the variant data.
+ *
+ * Returns: (transfer full): A new #GBytes representing the variant data
+ *
+ * Since: 2.36
+ */
+GBytes *
+g_variant_get_data_as_bytes (GVariant *value)
+{
+  gconstpointer data;
+  GBytes *bytes;
+  gsize size;
+  gconstpointer bytes_data;
+  gsize bytes_size;
+
+  data = g_variant_get_serialised (value, &bytes, &size);
+  bytes_data = g_bytes_get_data (bytes, &bytes_size);
+
+  /* Try to reuse the GBytes held internally by GVariant, if it exists
+   * and is covering exactly the correct range.
+   */
+  if (data == bytes_data && size == bytes_size)
+    return g_bytes_ref (bytes);
+
+  /* See g_variant_get_data() about why it can return NULL... */
+  else if (data == NULL)
+    return g_bytes_new_take (g_malloc0 (size), size);
+
+  /* Otherwise, make a new GBytes with reference to the old. */
+  else
+    return g_bytes_new_with_free_func (data, size, (GDestroyNotify) g_bytes_unref, g_bytes_ref (bytes));
 }
 
 /* Epilogue {{{1 */
diff --git a/glib/tests/gvariant.c b/glib/tests/gvariant.c
index b68c583..d42bdfb 100644
--- a/glib/tests/gvariant.c
+++ b/glib/tests/gvariant.c
@@ -4497,10 +4497,13 @@ test_gbytes (void)
 {
   GVariant *a;
   GVariant *tuple;
+  GVariant *b;
+  GVariant *c;
   GBytes *bytes;
   GBytes *bytes2;
   const guint8 values[5] = { 1, 2, 3, 4, 5 };
   const guint8 *elts;
+  gchar *tmp;
   gsize n_elts;
   gint i;
 
@@ -4531,6 +4534,41 @@ test_gbytes (void)
   g_bytes_unref (bytes2);
   g_variant_unref (a);
   g_variant_unref (tuple);
+
+  /* Feed in some non-normal data... make sure it's aligned.
+   *
+   * Here we have an array of three elements.  The first and last are
+   * normal ints ('iiii') and array-of-bytes data ('ayay').  The middle
+   * element is zero-bytes wide, which will present a problem when
+   * fetching the fixed-size integer out of it.
+   * */
+  tmp = g_strdup ("iiiiayayiiiisayay\x08\x08\x10");
+  bytes = g_bytes_new_take (tmp, strlen (tmp));
+  a = g_variant_new_from_bytes (G_VARIANT_TYPE ("a(iay)"), bytes, FALSE);
+  g_bytes_unref (bytes);
+
+  /* The middle tuple is zero bytes */
+  b = g_variant_get_child_value (a, 1);
+  g_assert_cmpint (g_variant_get_size (b), ==, 0);
+
+  /* But we're going to pull a 4-byte child out of it... */
+  c = g_variant_get_child_value (b, 0);
+  g_assert_cmpint (g_variant_get_size (c), ==, 4);
+
+  /* g_variant_get_data() is allowed to fail in this case.
+   * NB: if someone finds a way to avoid this then that's fine too...
+   */
+  g_assert (g_variant_get_data (c) == NULL);
+
+  /* but since it's four bytes, it ought to have data... */
+  bytes = g_variant_get_data_as_bytes (c);
+  g_assert_cmpint (g_bytes_get_size (bytes), ==, 4);
+  g_assert (memcmp (g_bytes_get_data (bytes, NULL), "\0\0\0\0", 4) == 0);
+  g_bytes_unref (bytes);
+
+  g_variant_unref (c);
+  g_variant_unref (b);
+  g_variant_unref (a);
 }
 
 typedef struct {