Bug 21394 - [PATCH] Improved _dbus_keyring_validate_context()
Summary: [PATCH] Improved _dbus_keyring_validate_context()
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: lowest enhancement
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2009-04-24 14:23 UTC by Roberto Guido
Modified: 2014-09-23 15:32 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Improved _dbus_keyring_validate_context() and introduction of _dbus_string_find_char_from_sorted_array() (7.70 KB, patch)
2009-04-24 14:23 UTC, Roberto Guido
Details | Splinter Review

Description Roberto Guido 2009-04-24 14:23:00 UTC
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 1 Chengwei Yang 2013-12-04 04:29:19 UTC
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 2 Chengwei Yang 2013-12-04 04:50:09 UTC
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.
Comment 3 Simon McVittie 2013-12-04 12:39:06 UTC
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.