Bug 30350 - Implement dbus_message_iter_get_n_elements
Summary: Implement dbus_message_iter_get_n_elements
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-09-23 10:26 UTC by Christian Dywan
Modified: 2015-05-13 17:50 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Implement dbus_message_iter_get_n_elements (4.31 KB, patch)
2010-09-23 10:32 UTC, Christian Dywan
Details | Splinter Review
Implement dbus_message_iter_get_element_count (4.32 KB, patch)
2010-09-23 11:36 UTC, Christian Dywan
Details | Splinter Review
Implement dbus_message_iter_get_element_count #2 (6.58 KB, patch)
2010-10-01 02:30 UTC, Christian Dywan
Details | Splinter Review
Implement dbus_message_iter_get_element_count #3 (6.81 KB, patch)
2010-10-04 03:23 UTC, Christian Dywan
Details | Splinter Review
Implement dbus_message_iter_get_element_count (6.82 KB, patch)
2013-08-27 17:37 UTC, Simon McVittie
Details | Splinter Review
Fix whitespace as per Havoc's review (in 2010) (899 bytes, patch)
2013-08-27 17:39 UTC, Simon McVittie
Details | Splinter Review
Avoid reading beyond the length of a variable (1.72 KB, patch)
2013-08-27 17:39 UTC, Simon McVittie
Details | Splinter Review
Avoid reading beyond the length of a variable (1.72 KB, patch)
2013-08-27 18:20 UTC, Simon McVittie
Details | Splinter Review

Description Christian Dywan 2010-09-23 10:26:13 UTC
At the moment it is impossible to determine how many elements an array contains without iterating over it. There is a @todo remark about it in the sources.

The motivating use case here is that you want to allocate memory in advance before filling it with elements, for which you need to know the number of elements.
Comment 1 Christian Dywan 2010-09-23 10:32:02 UTC
Created attachment 38916 [details] [review]
Implement dbus_message_iter_get_n_elements

Included in the patch is a simple test case and the documentation of dbus_message_iter_get_array_len now refers to the new function.
Comment 2 Thiago Macieira 2010-09-23 11:18:00 UTC
I agree, but I suggest calling this dbus_message_iter_get_element_count instead. "get_n_elements" sounds like you pass an n and you get that many elements.
Comment 3 Christian Dywan 2010-09-23 11:36:52 UTC
Created attachment 38917 [details] [review]
Implement dbus_message_iter_get_element_count

No strong opinion personally. Updated patch with the new name.
Comment 4 Havoc Pennington 2010-09-23 13:19:35 UTC
Review of attachment 38917 [details] [review]:

::: dbus/dbus-message-util.c
@@ +1415,3 @@
+  dbus_message_iter_init (message, &iter);
+  _dbus_assert (dbus_message_iter_get_element_count (&iter) == 3);
+  dbus_message_unref (message);

Should probably test a couple of fixed-size types also, such as byte and int32.

actually using append_basic you can likely write a generic test that runs on any basic type and then call it with different typecodes.

Testing an array of structs or array of arrays would not be bad either.

::: dbus/dbus-message.c
@@ +2180,3 @@
+ * Returns the number of elements in the array-typed value pointed
+ * to by the iterator.
+ *

I think it would be good to note here that it's O(n) and therefore kind of a bad idea to use.

Well, really it should not be O(n) for fixed-size-element arrays... I think if we're going to have this it should probably use the array len (see deprecated function below) and divide by sizeof(element).
There may be a get-size-of-fixed-type utility function floating in the code somewhere, I don't remember.
Comment 5 Christian Dywan 2010-10-01 02:30:45 UTC
Created attachment 39098 [details] [review]
Implement dbus_message_iter_get_element_count #2

(In reply to comment #4)
> Should probably test a couple of fixed-size types also, such as byte and int32.
> Testing an array of structs or array of arrays would not be bad either.

I made it loop through the basic types now, and also test an array of structures.

> I think it would be good to note here that it's O(n) and therefore kind of a
> bad idea to use.
> Well, really it should not be O(n) for fixed-size-element arrays

I added a special-case for fixed-size arrays and documented it accordingly.
Comment 6 Havoc Pennington 2010-10-02 20:36:11 UTC
Review of attachment 39098 [details] [review]:

looks good, some nitpicks.

::: dbus/dbus-message-util.c
@@ +1418,3 @@
+      dbus_message_iter_open_container (&iter, DBUS_TYPE_ARRAY,
+                                        basic_types + i, &array_iter);
+      for (some = 0; some < 3; some++)

this for() could use braces around the body

@@ +1427,3 @@
+      _dbus_assert (dbus_message_iter_get_element_count (&iter) == some);
+      dbus_message_unref (message);
+      basic_types[i] = '\0';

I think some compilers or some gcc settings get upset about assigning to string literals.

::: dbus/dbus-message.c
@@ +2180,3 @@
+ * Returns the number of elements in the array-typed value pointed
+ * to by the iterator.
+ * Note that this function is O(1) for fixed-size arrays but O(n)

"arrays of fixed-size types" might be clearer

@@ +2181,3 @@
+ * to by the iterator.
+ * Note that this function is O(1) for fixed-size arrays but O(n)
+ * otherwise, so it may be a bad idea to use it.

"O(n) for arrays of variable-length types such as strings"

@@ +2189,3 @@
+{
+  DBusMessageRealIter *real = (DBusMessageRealIter *)iter;
+  int atype = _dbus_type_reader_get_current_type(&real->u.reader);

this assignment should be dropped down below the return_val_if_fail that checks validity of 'real'

@@ +2205,3 @@
+      return total_len / alignment;
+    }
+

clearer to then do "else" here I think with the non-fixed case
Comment 7 Christian Dywan 2010-10-04 03:23:25 UTC
Created attachment 39150 [details] [review]
Implement dbus_message_iter_get_element_count #3

(In reply to comment #6)
> Review of attachment 39098 [details] [review]:

I used an allocated signature string in the unit tests now and avoided the int atype declaration. I also incorporated the stylistic and rethorical suggestions.
Comment 8 Havoc Pennington 2010-10-04 11:17:05 UTC
Review of attachment 39150 [details] [review]:

This looks ready to commit to me, thanks

::: dbus/dbus-message.c
@@ +2187,3 @@
+ * @returns the number of elements in the array
+ */
+int dbus_message_iter_get_element_count (DBusMessageIter* iter)

should linebreak after "int" here I missed on first time through the patch.
Maybe someone can just fix that when committing though.
Comment 9 Havoc Pennington 2010-10-04 11:17:13 UTC
Review of attachment 39150 [details] [review]:

This looks ready to commit to me, thanks

::: dbus/dbus-message.c
@@ +2187,3 @@
+ * @returns the number of elements in the array
+ */
+int dbus_message_iter_get_element_count (DBusMessageIter* iter)

should linebreak after "int" here I missed on first time through the patch.
Maybe someone can just fix that when committing though.
Comment 10 Simon McVittie 2013-08-27 17:37:45 UTC
Created attachment 84731 [details] [review]
Implement dbus_message_iter_get_element_count

From: Christian Dywan

According unit tests are added to _dbus_message_test.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=30350
Reviewed-by: Havoc Pennington <hp@pobox.com>
Comment 11 Simon McVittie 2013-08-27 17:39:06 UTC
Created attachment 84732 [details] [review]
Fix whitespace as per Havoc's review (in 2010)

---

Not going to bother with review for this one :-P
Comment 12 Simon McVittie 2013-08-27 17:39:29 UTC
Created attachment 84733 [details] [review]
Avoid reading beyond the length of a variable

Appending &some as DBUS_TYPE_INT64, DBUS_TYPE_UINT64 or DBUS_TYPE_DOUBLE,
where "some" is an int, reads beyond the bounds of that variable.
Use a zero-filled DBusBasicValue instead.
Comment 13 Simon McVittie 2013-08-27 17:40:01 UTC
(In reply to comment #12)
> Avoid reading beyond the length of a variable

I'm OK with applying Christian's patch as long as we also apply this.
Comment 14 Simon McVittie 2013-08-27 18:20:00 UTC
Created attachment 84738 [details] [review]
Avoid reading beyond the length of a variable

Appending &some as DBUS_TYPE_INT64, DBUS_TYPE_UINT64 or DBUS_TYPE_DOUBLE,
where "some" is an int, reads beyond the bounds of that variable.
Use a zero-filled DBusBasicValue instead.

---

v2: sorry, forgot to --amend.
Comment 15 patrick dakon 2014-12-22 21:19:47 UTC
Comment on attachment 38917 [details] [review]
Implement dbus_message_iter_get_element_count

>From b9c996ca75d3f7434449cfa60629e3a285910f1b Mon Sep 17 00:00:00 2001
>From: Christian Dywan <christian.dywan@lanedo.com>
>Date: Thu, 23 Sep 2010 19:22:53 +0200
>Subject: [PATCH] Implement dbus_message_iter_get_element_count
>
>See https://bugs.freedesktop.org/show_bug.cgi?id=30350
>---
> dbus/dbus-message-util.c |   20 ++++++++++++++++++++
> dbus/dbus-message.c      |   34 +++++++++++++++++++++++++++-------
> dbus/dbus-message.h      |    3 +++
> 3 files changed, 50 insertions(+), 7 deletions(-)
>
>diff --git a/dbus/dbus-message-util.c b/dbus/dbus-message-util.c
>index f972c8a..9a502f7 100644
>--- a/dbus/dbus-message-util.c
>+++ b/dbus/dbus-message-util.c
>@@ -1396,6 +1396,26 @@ _dbus_message_test (const char *test_data_dir)
>   check_memleaks ();
>   _dbus_check_fdleaks();
> 
>+  /* Test enumeration of array elements */
>+  message = dbus_message_new_method_call ("de.ende.test",
>+                                          "/de/ende/test",
>+                                          "de.ende.Test",
>+                                          "ArtistName");
>+  _dbus_assert (message != NULL);
>+  dbus_message_iter_init_append (message, &iter);
>+  dbus_message_iter_open_container (&iter, DBUS_TYPE_ARRAY,
>+                                    DBUS_TYPE_STRING_AS_STRING, &array_iter);
>+  s = "SomeThingToSay";
>+  dbus_message_iter_append_basic (&array_iter, DBUS_TYPE_STRING, &s);
>+  s = "MoreHumbugToCome";
>+  dbus_message_iter_append_basic (&array_iter, DBUS_TYPE_STRING, &s);
>+  s = "WerewolvesMinotaursKirins";
>+  dbus_message_iter_append_basic (&array_iter, DBUS_TYPE_STRING, &s);
>+  dbus_message_iter_close_container (&iter, &array_iter);
>+  dbus_message_iter_init (message, &iter);
>+  _dbus_assert (dbus_message_iter_get_element_count (&iter) == 3);
>+  dbus_message_unref (message);
>+
>   /* Check that we can abandon a container */
>   message = dbus_message_new_method_call ("org.freedesktop.DBus.TestService",
> 		  			  "/org/freedesktop/TestPath",
>diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
>index b19697e..bd11e18 100644
>--- a/dbus/dbus-message.c
>+++ b/dbus/dbus-message.c
>@@ -2177,20 +2177,40 @@ dbus_message_iter_get_basic (DBusMessageIter  *iter,
> }
> 
> /**
>+ * Returns the number of elements in the array-typed value pointed
>+ * to by the iterator.
>+ *
>+ * @param iter the iterator
>+ * @returns the number of elements in the array
>+ */
>+int dbus_message_iter_get_element_count (DBusMessageIter* iter)
>+{
>+  DBusMessageRealIter *real = (DBusMessageRealIter *)iter;
>+  DBusTypeReader array;
>+  int n_elements = 0;
>+
>+  _dbus_return_val_if_fail (_dbus_message_iter_check (real), 0);
>+  _dbus_return_val_if_fail (_dbus_type_reader_get_current_type (&real->u.reader)
>+                            == DBUS_TYPE_ARRAY, 0);
>+
>+  _dbus_type_reader_recurse (&real->u.reader, &array);
>+  while (_dbus_type_reader_get_current_type (&array) != DBUS_TYPE_INVALID)
>+    {
>+      ++n_elements;
>+      _dbus_type_reader_next (&array);
>+    }
>+  return n_elements;
>+}
>+
>+/**
>  * Returns the number of bytes in the array as marshaled in the wire
>  * protocol. The iterator must currently be inside an array-typed
>  * value.
>  *
>  * This function is deprecated on the grounds that it is stupid.  Why
>  * would you want to know how many bytes are in the array as marshaled
>- * in the wire protocol?  For now, use the n_elements returned from
>- * dbus_message_iter_get_fixed_array() instead, or iterate over the
>- * array values and count them.
>+ * in the wire protocol?  Use dbus_message_iter_get_element_count() instead.
>  *
>- * @todo introduce a variant of this get_n_elements that returns
>- * the number of elements, though with a non-fixed array it will not
>- * be very efficient, so maybe it's not good.
>- * 
>  * @param iter the iterator
>  * @returns the number of bytes in the array
>  */
>diff --git a/dbus/dbus-message.h b/dbus/dbus-message.h
>index 5500492..fa1b2d4 100644
>--- a/dbus/dbus-message.h
>+++ b/dbus/dbus-message.h
>@@ -226,6 +226,9 @@ void        dbus_message_iter_recurse          (DBusMessageIter *iter,
> DBUS_EXPORT
> void        dbus_message_iter_get_basic        (DBusMessageIter *iter,
>                                                 void            *value);
>+DBUS_EXPORT
>+int         dbus_message_iter_get_element_count(DBusMessageIter *iter);
>+
> #ifndef DBUS_DISABLE_DEPRECATED
> /* This function returns the wire protocol size of the array in bytes,
>  * you do not want to know that probably
>-- 
>1.7.2.3
>
Comment 16 Simon McVittie 2015-05-13 17:50:52 UTC
(In reply to Simon McVittie from comment #13)
> I'm OK with applying Christian's patch as long as we also apply [...]

(In reply to Simon McVittie from comment #14)
> Avoid reading beyond the length of a variable

This has been waiting for review nearly 2 years, as a fix for a nearly 5 year old patch. That's quite long enough; I'm just going to apply it.

Fixed in git for 1.9.16.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.