2005-01-17 Havoc Pennington <hp@redhat.com>

        * Throughout, align variant bodies according to the contained
	type, rather than always to 8. Should save a fair bit of space in
	message headers.

	* dbus/dbus-marshal-validate.c (_dbus_validate_body_with_reason):
	fix handling of case where p == end

	* doc/TODO: remove the dbus_bool_t item and variant alignment items

7 files changed, 80 insertions, 71 deletions

diff --git a/ChangeLog b/ChangeLog
index 2629104d..0d7c8015 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2005-01-17  Havoc Pennington  <hp@redhat.com>
 
+        * Throughout, align variant bodies according to the contained
+	type, rather than always to 8. Should save a fair bit of space in
+	message headers.
+	
+	* dbus/dbus-marshal-validate.c (_dbus_validate_body_with_reason):
+	fix handling of case where p == end
+
+	* doc/TODO: remove the dbus_bool_t item and variant alignment items
+
+2005-01-17  Havoc Pennington  <hp@redhat.com>
+
 	* dbus/dbus-types.h: hardcode dbus_bool_t to 32 bits
 
 	* Throughout: modify DBUS_TYPE_BOOLEAN to be a 32-bit type instead
diff --git a/dbus/dbus-marshal-basic.c b/dbus/dbus-marshal-basic.c
index fce935fa..5cb43b88 100644
--- a/dbus/dbus-marshal-basic.c
+++ b/dbus/dbus-marshal-basic.c
@@ -1389,6 +1389,30 @@ _dbus_verbose_bytes_of_string (const DBusString    *str,
   _dbus_verbose_bytes (d, len, start);
 }
 
+/**
+ * Get the first type in the signature. The difference between this
+ * and just getting the first byte of the signature is that you won't
+ * get DBUS_STRUCT_BEGIN_CHAR, you'll get DBUS_TYPE_STRUCT
+ * instead.
+ *
+ * @param str string containing signature
+ * @param pos where the signature starts
+ * @returns the first type in the signature
+ */
+int
+_dbus_first_type_in_signature (const DBusString *str,
+                               int               pos)
+{
+  unsigned char t;
+
+  t = _dbus_string_get_byte (str, pos);
+
+  if (t == DBUS_STRUCT_BEGIN_CHAR)
+    return DBUS_TYPE_STRUCT;
+  else
+    return t;
+}
+
 /** @} */
 
 #ifdef DBUS_BUILD_TESTS
diff --git a/dbus/dbus-marshal-basic.h b/dbus/dbus-marshal-basic.h
index f4a8c2fa..d8ded1ca 100644
--- a/dbus/dbus-marshal-basic.h
+++ b/dbus/dbus-marshal-basic.h
@@ -215,6 +215,7 @@ dbus_bool_t   _dbus_type_is_container         (int               typecode);
 dbus_bool_t   _dbus_type_is_fixed             (int               typecode);
 const char*   _dbus_type_to_string            (int               typecode);
 
-
+int           _dbus_first_type_in_signature   (const DBusString *str,
+                                               int               pos);
 
 #endif /* DBUS_MARSHAL_BASIC_H */
diff --git a/dbus/dbus-marshal-recursive-util.c b/dbus/dbus-marshal-recursive-util.c
index 28797099..e257a51c 100644
--- a/dbus/dbus-marshal-recursive-util.c
+++ b/dbus/dbus-marshal-recursive-util.c
@@ -31,20 +31,6 @@
 #include <stdio.h>
 #include <stdlib.h>
 
-static int
-first_type_in_signature (const DBusString *str,
-                         int               pos)
-{
-  unsigned char t;
-
-  t = _dbus_string_get_byte (str, pos);
-
-  if (t == DBUS_STRUCT_BEGIN_CHAR)
-    return DBUS_TYPE_STRUCT;
-  else
-    return t;
-}
-
 /* Whether to do the OOM stuff (only with other expensive tests) */
 #define TEST_OOM_HANDLING 0
 /* We do start offset 0 through 9, to get various alignment cases. Still this
@@ -2678,7 +2664,7 @@ array_write_value (TestTypeNode   *node,
                              &element_signature))
     goto oom;
 
-  element_type = first_type_in_signature (&element_signature, 0);
+  element_type = _dbus_first_type_in_signature (&element_signature, 0);
 
   if (!_dbus_type_writer_recurse (writer, DBUS_TYPE_ARRAY,
                                   &element_signature, 0,
diff --git a/dbus/dbus-marshal-recursive.c b/dbus/dbus-marshal-recursive.c
index e25fe249..e98d1c35 100644
--- a/dbus/dbus-marshal-recursive.c
+++ b/dbus/dbus-marshal-recursive.c
@@ -123,24 +123,10 @@ struct DBusTypeReaderClass
 };
 
 static int
-first_type_in_signature (const DBusString *str,
-                         int               pos)
-{
-  unsigned char t;
-
-  t = _dbus_string_get_byte (str, pos);
-
-  if (t == DBUS_STRUCT_BEGIN_CHAR)
-    return DBUS_TYPE_STRUCT;
-  else
-    return t;
-}
-
-static int
 element_type_get_alignment (const DBusString *str,
                             int               pos)
 {
-  return _dbus_type_get_alignment (first_type_in_signature (str, pos));
+  return _dbus_type_get_alignment (_dbus_first_type_in_signature (str, pos));
 }
 
 static void
@@ -265,7 +251,7 @@ array_reader_recurse (DBusTypeReader *sub,
                  sub->u.array.start_pos,
                  sub->array_len_offset,
                  array_reader_get_array_len (sub),
-                 _dbus_type_to_string (first_type_in_signature (sub->type_str,
+                 _dbus_type_to_string (_dbus_first_type_in_signature (sub->type_str,
                                                                 sub->type_pos)));
 #endif
 }
@@ -275,6 +261,7 @@ variant_reader_recurse (DBusTypeReader *sub,
                         DBusTypeReader *parent)
 {
   int sig_len;
+  int contained_alignment;
 
   base_reader_recurse (sub, parent);
 
@@ -289,7 +276,10 @@ variant_reader_recurse (DBusTypeReader *sub,
 
   sub->value_pos = sub->type_pos + sig_len + 1;
 
-  sub->value_pos = _DBUS_ALIGN_VALUE (sub->value_pos, 8);
+  contained_alignment = _dbus_type_get_alignment (_dbus_first_type_in_signature (sub->type_str,
+                                                                           sub->type_pos));
+  
+  sub->value_pos = _DBUS_ALIGN_VALUE (sub->value_pos, contained_alignment);
 
 #if RECURSIVE_MARSHAL_READ_TRACE
   _dbus_verbose ("    type reader %p variant containing '%s'\n",
@@ -429,7 +419,7 @@ base_reader_next (DBusTypeReader *reader,
       {
         if (!reader->klass->types_only)
           _dbus_marshal_skip_array (reader->value_str,
-                                    first_type_in_signature (reader->type_str,
+                                    _dbus_first_type_in_signature (reader->type_str,
                                                              reader->type_pos + 1),
                                     reader->byte_order,
                                     &reader->value_pos);
@@ -502,7 +492,7 @@ array_reader_next (DBusTypeReader *reader,
   _dbus_assert (reader->value_pos < end_pos);
   _dbus_assert (reader->value_pos >= reader->u.array.start_pos);
 
-  switch (first_type_in_signature (reader->type_str,
+  switch (_dbus_first_type_in_signature (reader->type_str,
                                    reader->type_pos))
     {
     case DBUS_TYPE_STRUCT:
@@ -527,7 +517,7 @@ array_reader_next (DBusTypeReader *reader,
     case DBUS_TYPE_ARRAY:
       {
         _dbus_marshal_skip_array (reader->value_str,
-                                  first_type_in_signature (reader->type_str,
+                                  _dbus_first_type_in_signature (reader->type_str,
                                                            reader->type_pos + 1),
                                   reader->byte_order,
                                   &reader->value_pos);
@@ -807,7 +797,7 @@ _dbus_type_reader_get_current_type (const DBusTypeReader *reader)
        (* reader->klass->check_finished) (reader)))
     t = DBUS_TYPE_INVALID;
   else
-    t = first_type_in_signature (reader->type_str,
+    t = _dbus_first_type_in_signature (reader->type_str,
                                  reader->type_pos);
 
   _dbus_assert (t != DBUS_STRUCT_END_CHAR);
@@ -837,7 +827,7 @@ _dbus_type_reader_get_element_type (const DBusTypeReader  *reader)
 
   _dbus_assert (_dbus_type_reader_get_current_type (reader) == DBUS_TYPE_ARRAY);
 
-  element_type = first_type_in_signature (reader->type_str,
+  element_type = _dbus_first_type_in_signature (reader->type_str,
                                           reader->type_pos + 1);
 
   return element_type;
@@ -932,7 +922,7 @@ _dbus_type_reader_read_fixed_multi (const DBusTypeReader  *reader,
   _dbus_assert (!reader->klass->types_only);
   _dbus_assert (reader->klass == &array_reader_class);
 
-  element_type = first_type_in_signature (reader->type_str,
+  element_type = _dbus_first_type_in_signature (reader->type_str,
                                           reader->type_pos);
 
   _dbus_assert (element_type != DBUS_TYPE_INVALID); /* why we don't use get_current_type() */
@@ -989,7 +979,7 @@ _dbus_type_reader_recurse (DBusTypeReader *reader,
 {
   int t;
 
-  t = first_type_in_signature (reader->type_str, reader->type_pos);
+  t = _dbus_first_type_in_signature (reader->type_str, reader->type_pos);
 
   switch (t)
     {
@@ -1649,7 +1639,7 @@ writer_recurse_init_and_check (DBusTypeWriter *writer,
     {
       int expected;
 
-      expected = first_type_in_signature (writer->type_str, writer->type_pos);
+      expected = _dbus_first_type_in_signature (writer->type_str, writer->type_pos);
 
       if (expected != sub->container_type)
         {
@@ -1937,7 +1927,7 @@ writer_recurse_array (DBusTypeWriter   *writer,
 /* Variant value will normally have:
  *   1 byte signature length not including nul
  *   signature typecodes (nul terminated)
- *   padding to 8-boundary
+ *   padding to alignment of contained type
  *   body according to signature
  *
  * The signature string can only have a single type
@@ -1945,21 +1935,12 @@ writer_recurse_array (DBusTypeWriter   *writer,
  *
  * So a typical variant type with the integer 3 will have these
  * octets:
- *   0x1 'i' '\0' [padding to 8-boundary] 0x0 0x0 0x0 0x3
- *
- * For an array of 4-byte types stuffed into variants, the padding to
- * 8-boundary is only the 1 byte that is required for the 4-boundary
- * anyhow for all array elements after the first one. And for single
- * variants in isolation, wasting a few bytes is hardly a big deal.
+ *   0x1 'i' '\0' [1 byte padding to alignment boundary] 0x0 0x0 0x0 0x3
  *
  * The main world of hurt for writing out a variant is that the type
  * string is the same string as the value string. Which means
  * inserting to the type string will move the value_pos; and it means
  * that inserting to the type string could break type alignment.
- *
- * This type alignment issue is why the body of the variant is always
- * 8-aligned. Then we know that re-8-aligning the start of the body
- * will always correctly align the full contents of the variant type.
  */
 static dbus_bool_t
 writer_recurse_variant (DBusTypeWriter   *writer,
@@ -1968,6 +1949,8 @@ writer_recurse_variant (DBusTypeWriter   *writer,
                         int               contained_type_len,
                         DBusTypeWriter   *sub)
 {
+  int contained_alignment;
+  
   if (writer->enabled)
     {
       /* Allocate space for the worst case, which is 1 byte sig
@@ -2018,12 +2001,14 @@ writer_recurse_variant (DBusTypeWriter   *writer,
 
   sub->value_pos += 1;
 
+  contained_alignment = _dbus_type_get_alignment (_dbus_first_type_in_signature (contained_type, contained_type_start));
+  
   if (!_dbus_string_insert_bytes (sub->value_str,
                                   sub->value_pos,
-                                  _DBUS_ALIGN_VALUE (sub->value_pos, 8) - sub->value_pos,
+                                  _DBUS_ALIGN_VALUE (sub->value_pos, contained_alignment) - sub->value_pos,
                                   '\0'))
     _dbus_assert_not_reached ("should not have failed to insert alignment padding for variant body");
-  sub->value_pos = _DBUS_ALIGN_VALUE (sub->value_pos, 8);
+  sub->value_pos = _DBUS_ALIGN_VALUE (sub->value_pos, contained_alignment);
 
   return TRUE;
 }
diff --git a/dbus/dbus-marshal-validate.c b/dbus/dbus-marshal-validate.c
index 05148604..285f1efb 100644
--- a/dbus/dbus-marshal-validate.c
+++ b/dbus/dbus-marshal-validate.c
@@ -304,7 +304,10 @@ validate_body_helper (DBusTypeReader       *reader,
 
         case DBUS_TYPE_VARIANT:
           {
-            /* 1 byte sig len, sig typecodes, align to 8-boundary, values. */
+            /* 1 byte sig len, sig typecodes, align to
+             * contained-type-boundary, values.
+             */
+
             /* In addition to normal signature validation, we need to be sure
              * the signature contains only a single (possibly container) type.
              */
@@ -312,6 +315,7 @@ validate_body_helper (DBusTypeReader       *reader,
             DBusString sig;
             DBusTypeReader sub;
             DBusValidity validity;
+            int contained_alignment;
 
             claimed_len = *p;
             ++p;
@@ -331,7 +335,9 @@ validate_body_helper (DBusTypeReader       *reader,
               return DBUS_INVALID_VARIANT_SIGNATURE_MISSING_NUL;
             ++p;
 
-            a = _DBUS_ALIGN_ADDRESS (p, 8);
+            contained_alignment = _dbus_type_get_alignment (_dbus_first_type_in_signature (&sig, 0));
+            
+            a = _DBUS_ALIGN_ADDRESS (p, contained_alignment);
             if (a > end)
               return DBUS_INVALID_NOT_ENOUGH_DATA;
             while (p != a)
@@ -460,16 +466,19 @@ _dbus_validate_body_with_reason (const DBusString *expected_signature,
   validity = validate_body_helper (&reader, byte_order, TRUE, p, end, &p);
   if (validity != DBUS_VALID)
     return validity;
-
-  if (p < end)
+  
+  if (bytes_remaining)
     {
-      if (bytes_remaining)
-        *bytes_remaining = end - p;
-      else
-        return DBUS_INVALID_TOO_MUCH_DATA;
+      *bytes_remaining = end - p;
+      return DBUS_VALID;
+    }
+  else if (p < end)
+    return DBUS_INVALID_TOO_MUCH_DATA;
+  else
+    {
+      _dbus_assert (p == end);
+      return DBUS_VALID;
     }
-
-  return DBUS_VALID;
 }
 
 /**
diff --git a/doc/TODO b/doc/TODO
index f670df80..c688583e 100644
--- a/doc/TODO
+++ b/doc/TODO
@@ -38,13 +38,6 @@ Important for 1.0
    (though they are kind of a pita to pass in as size_t with the 
     varargs, so maybe not - what does glib do with g_object_get()?)
 
- - it's probably obnoxious that reading/writing bools doesn't return
-   dbus_bool_t; solution is probably to change bool to 32 bits on the
-   wire
-
- - maybe change and don't align variant bodies to 8-boundary, it uses
-   up lots of space in a typical header
-
  - rename the service thing. unique service names (":1") and well-known
    ("org.foo.bar") should have different names probably; something like 
    "address" for the unique and "alias" for the well-known, or