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.
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.
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.
Created attachment 38917 [details] [review] Implement dbus_message_iter_get_element_count No strong opinion personally. Updated patch with the new name.
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.
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.
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
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.
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.
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>
Created attachment 84732 [details] [review] Fix whitespace as per Havoc's review (in 2010) --- Not going to bother with review for this one :-P
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.
(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.
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 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 >
(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.