Created attachment 25119 [details] [review] Improved _dbus_keyring_validate_context() and introduction of _dbus_string_find_char_from_sorted_array() This patch improves _dbus_keyring_validate_context() introducing a new function for DBusString, able to check for a given set of chars all in a single round. Specific tests are also added. Solves a TODO found in code.
Comment on attachment 25119 [details] [review] Improved _dbus_keyring_validate_context() and introduction of _dbus_string_find_char_from_sorted_array() Review of attachment 25119 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-keyring.c @@ +883,5 @@ > + _dbus_verbose ("context contains a dot\n"); > + break; > + case '/': > + _dbus_verbose ("context contains a slash\n"); > + break; case '\\': is missing. ::: dbus/dbus-string-util.c @@ +690,5 @@ > + _dbus_assert_not_reached ("Did find 'abc'"); > + _dbus_assert (i == -1); > + > + if (!_dbus_string_find_char_from_sorted_array(&str, 0, _dbus_string_get_length (&str), "abcl", &i)) > + _dbus_assert_not_reached ("Didn't find 'l'"); Should be "Didn't find 'abcl'". @@ +694,5 @@ > + _dbus_assert_not_reached ("Didn't find 'l'"); > + _dbus_assert (i == 2); > + > + if (!_dbus_string_find_char_from_sorted_array(&str, 0, _dbus_string_get_length (&str), "cel", &i)) > + _dbus_assert_not_reached ("Didn't find 'e'"); ditto. @@ +702,5 @@ > + _dbus_assert_not_reached ("Did find 'abc'"); > + _dbus_assert (i == -1); > + > + if (!_dbus_string_find_char_from_sorted_array(&str, 2, _dbus_string_get_length (&str) - 1, "abcl", &i)) > + _dbus_assert_not_reached ("Didn't find 'l'"); ditto. @@ +706,5 @@ > + _dbus_assert_not_reached ("Didn't find 'l'"); > + _dbus_assert (i == 2); > + > + if (_dbus_string_find_char_from_sorted_array(&str, 2, _dbus_string_get_length (&str) - 1, "cem", &i)) > + _dbus_assert_not_reached ("Did find 'e'"); ditto. @@ +708,5 @@ > + > + if (_dbus_string_find_char_from_sorted_array(&str, 2, _dbus_string_get_length (&str) - 1, "cem", &i)) > + _dbus_assert_not_reached ("Did find 'e'"); > + _dbus_assert (i == -1); > + In fact, the above test cases are basically two different cases. We need some more like: find single char, escape chars, up lower chars and etc. ::: dbus/dbus-string.c @@ +2058,5 @@ > + /* '\0' is found in every string */ > + if (*chars == '\0') > + { > + if (found) > + *found = start; why not the last char of string? which is the real '\0'. Or just do _dbus_assert (*chars != '\0'); @@ +2067,5 @@ > + > +#ifndef DBUS_DISABLE_ASSERT > + for ( i = 0; i < last_char_pos; i++ ) > + { > + _dbus_assert (chars[i] < chars[i + 1]); this is the core part of this function, should not depends on dbus assert. Needs to be ensure when assertion disabled.
Comment on attachment 25119 [details] [review] Improved _dbus_keyring_validate_context() and introduction of _dbus_string_find_char_from_sorted_array() Review of attachment 25119 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-string.c @@ +2027,5 @@ > + * Finds a char from an array sorted in ASCII order > + * in the string, starting from a certain position > + * up to another. > + * Returns #TRUE and filling in the byte index > + * where one of the char was found, if it was found. ", if it was found" seems redundant. @@ +2029,5 @@ > + * up to another. > + * Returns #TRUE and filling in the byte index > + * where one of the char was found, if it was found. > + * Returns #FALSE if any char is found. > + * Sets *found to -1 if none of the chars is found Returns #FALSE if none of chars was found and sets *found to -1 if its not NULL pointer.
What's wrong with the current implementation? If it isn't broken, we shouldn't fix it. The patch does delete this comment - * @todo this is the most inefficient implementation - * imaginable. but if it isn't invoked frequently enough that its performance matters, then I don't really care. Let's not optimize things that don't show up in benchmarks; our frequently-used code is bad enough (e.g. Bug #38288).
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.