|
Description
Chengwei Yang
2013-08-20 06:24:44 UTC
Created attachment 84302 [details] [review] [PATCH 01/10] Cleanup: polish verbose mode checking Created attachment 84303 [details] [review] [PATCH 02/10] Cleanup: polish inotify backend Created attachment 84304 [details] [review] [PATCH 03/10] Move function to the right place where it supposed to be used (merged) Created attachment 84305 [details] [review] [PATCH 04/10] DBusWatch: dbus_watch_get_socket deprecated the other two APIs Created attachment 84306 [details] [review] [PATCH 05/10] Cleanup: simplify assertion check Created attachment 84307 [details] [review] [PATCH 06/10] dbus-server.c: use the new _dbus_atomic_get function Created attachment 84308 [details] [review] [PATCH 07/10] dbus-server.c: trace all server ref count change events Created attachment 84309 [details] [review] [PATCH 08/10] Cleanup: remove temporary error Created attachment 84310 [details] [review] [PATCH 09/10] Fix reference doc in comments (merged) Created attachment 84311 [details] [review] [PATCH 10/10] Align behavior with document Comment on attachment 84302 [details] [review] [PATCH 01/10] Cleanup: polish verbose mode checking Review of attachment 84302 [details] [review]: ----------------------------------------------------------------- One logical change per commit, please, so I don't have to say "yes to this bit, no to this bit" so often :-) ::: bus/selinux.c @@ +936,4 @@ > void > bus_selinux_id_table_print (DBusHashTable *service_table) > { > +#if defined (DBUS_ENABLE_VERBOSE_MODE) && defined (HAVE_SELINUX) Sure @@ +961,5 @@ > */ > static void > bus_avc_print_stats (void) > { > +#if defined (DBUS_ENABLE_VERBOSE_MODE) && defined (HAVE_SELINUX) This is also OK @@ +1001,4 @@ > sidput (bus_sid); > bus_sid = SECSID_WILD; > > + bus_avc_print_stats (); and this ::: bus/signals.c @@ +123,5 @@ > */ > static char* > match_rule_to_string (BusMatchRule *rule) > { > +#ifdef DBUS_ENABLE_VERBOSE_MODE I'm not so sure about this stuff with match_rule_to_string() - dbus_free() can't just be inlined like the rest. I'd prefer to leave this as it is. Comment on attachment 84303 [details] [review] [PATCH 02/10] Cleanup: polish inotify backend Review of attachment 84303 [details] [review]: ----------------------------------------------------------------- ::: bus/dir-watch-inotify.c @@ +63,2 @@ > > + pid = _dbus_getpid (); Couldn't this, and the declaration of pid, even move into the {} with the kill()? @@ +68,5 @@ > else if (!ret) > _dbus_verbose ("Error reading inotify event: buffer too small\n"); > + else > + { > + _dbus_verbose ("Sending SIGHUP signal on reception of %d inotify event(s)\n", (int)ret); I'd prefer "%ld" and (long) ret, since (s)size_t is often larger than int. Comment on attachment 84304 [details] [review] [PATCH 03/10] Move function to the right place where it supposed to be used (merged) Review of attachment 84304 [details] [review]: ----------------------------------------------------------------- Yes Comment on attachment 84305 [details] [review] [PATCH 04/10] DBusWatch: dbus_watch_get_socket deprecated the other two APIs Review of attachment 84305 [details] [review]: ----------------------------------------------------------------- This API isn't amazingly well thought-out, but I'm not sure this patch really makes it much better. Fundamentally, we're not tracking enough information to get this right (see the FIXME). The problem is that on Unix files, pipes, sockets etc. are all small integers chosen from the same "space" (you can never have a pipe and a socket that are both fd 23), but on Windows, C runtime file descriptors (including pipes) are small integers, and Winsock sockets are a potentially-overlapping set of small integers. On Windows, it is possible to have a pipe or file that is fd 23, and a Winsock socket that is *also* 23. Comment on attachment 84306 [details] [review] [PATCH 05/10] Cleanup: simplify assertion check Review of attachment 84306 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +884,4 @@ > /* The message signature has already been checked for us, > * so let's just assert it's right. > */ > + _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY); OK @@ +1925,5 @@ > _dbus_verbose ("Driver got a method call: %s\n", name); > > /* security checks should have kept this from getting here */ > + _dbus_assert (dbus_message_get_sender (message) != NULL || > + strcmp (name, "Hello") == 0); OK ::: dbus/dbus-connection.c @@ +354,1 @@ > _dbus_atomic_inc (&filter->refcount); No, this one issues twice as many costly atomic operations. ::: dbus/dbus-memory.c @@ +706,4 @@ > check_guards (memory, TRUE); > if (memory) > { > + _dbus_assert (_dbus_atomic_get (&n_blocks_outstanding) >= 1); Also an extra atomic operation. @@ +719,4 @@ > if (memory) /* we guarantee it's safe to free (NULL) */ > { > #ifdef DBUS_ENABLE_EMBEDDED_TESTS > + _dbus_assert (_dbus_atomic_get (&n_blocks_outstanding) >= 1); Also here. ::: dbus/dbus-object-tree.c @@ +1006,4 @@ > static DBusObjectSubtree * > _dbus_object_subtree_ref (DBusObjectSubtree *subtree) > { > + _dbus_assert (_dbus_atomic_get (&subtree->refcount) > 0); And here. ::: dbus/dbus-server.c @@ +112,4 @@ > const DBusServerVTable *vtable, > const DBusString *address) > { > + _dbus_assert (_dbus_atomic_get (&server->refcount) == 0); And here. ::: dbus/dbus-string.c @@ +335,4 @@ > * disable asserts to profile, you don't get this destroyer > * of profiles. > */ > +#if defined (DBUS_ENABLE_EMBEDDED_TESTS) && !defined (DBUS_DISABLE_ASSERT) OK Comment on attachment 84307 [details] [review] [PATCH 06/10] dbus-server.c: use the new _dbus_atomic_get function Review of attachment 84307 [details] [review]: ----------------------------------------------------------------- No, I did this intentionally. Doing more atomic operations than we need to is expensive. ::: dbus/dbus-server.c @@ +694,4 @@ > > _dbus_return_val_if_fail (server != NULL, NULL); > > + old_refcount = _dbus_atomic_get (&server->refcount); No, this issues an extra atomic operation on a normal code path. I know the "undo" is an extra atomic op, but it's on a "can't happen" path anyway. Comment on attachment 84308 [details] [review] [PATCH 07/10] dbus-server.c: trace all server ref count change events Review of attachment 84308 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-server.c @@ +117,5 @@ > server->vtable = vtable; > _dbus_atomic_inc (&server->refcount); > + _dbus_server_trace_ref (server, 0, > + _dbus_atomic_get (&server->refcount), > + "init_base"); I would rather avoid the double-atomic here. @@ +776,4 @@ > _dbus_return_if_fail (server != NULL); > _dbus_return_if_fail (_dbus_atomic_get (&server->refcount) > 0); > > + dbus_server_ref (server); Hmm, maybe. I'll have to look at this bit in more detail later. Comment on attachment 84309 [details] [review] [PATCH 08/10] Cleanup: remove temporary error Review of attachment 84309 [details] [review]: ----------------------------------------------------------------- No, in all these cases, error might be NULL, so we can't look at *error. We really do need the "inner error" pattern - it's a common idiom in GLib too. Comment on attachment 84310 [details] [review] [PATCH 09/10] Fix reference doc in comments (merged) Review of attachment 84310 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 84311 [details] [review] [PATCH 10/10] Align behavior with document Review of attachment 84311 [details] [review]: ----------------------------------------------------------------- What practical effect does this have? Whenever the documentation and code disagree, there are two possibilities: either the code is wrong, or the documentation is wrong. Which will break fewest otherwise-correct programs? Created attachment 84503 [details] [review] [PATCH v2 1/7] Cleanup: polish verbose mode checking Created attachment 84504 [details] [review] [PATCH v2 2/7] Cleanup: polish inotify backend Created attachment 84505 [details] [review] [PATCH v2 3/7] Cleanup: simplify assertion check Created attachment 84507 [details] [review] [PATCH v2 4/7] Make sure the debug output ref is correct Created attachment 84508 [details] [review] [PATCH v2 5/7] Fix comment about atomic operations Created attachment 84509 [details] [review] [PATCH v2 6/7] Fix debug output about dbus server ref count Created attachment 84510 [details] [review] [PATCH v2 7/7] Align behavior with document Comment on attachment 84507 [details] [review] [PATCH v2 4/7] Make sure the debug output ref is correct Review of attachment 84507 [details] [review]: ----------------------------------------------------------------- No, this is not desirable, and can even produce misleading debug output. If two threads unref the connection at the same time, then the debug output will either look like unref: 6 -> 5 unref: 5 -> 4 which is fine, or unref: 5 -> 4 unref: 6 -> 5 which is slightly confusing until you realize that the first thread unreffed second, but printed its debug output first. However, unref: 5 -> 4 unref: 6 -> 4 is worse: it makes it look as though our atomic operations didn't even work correctly. (Also, it doubles the number of atomic ops.) Comment on attachment 84508 [details] [review] [PATCH v2 5/7] Fix comment about atomic operations Review of attachment 84508 [details] [review]: ----------------------------------------------------------------- Makes sense now, thanks. Comment on attachment 84510 [details] [review] [PATCH v2 7/7] Align behavior with document Review of attachment 84510 [details] [review]: ----------------------------------------------------------------- There are no callers where address_problem_type and _field don't have the same NULL'ness, and there shouldn't be. Since the intention seems to be that you pass either type-and-field, or just other, I think this should either have something like: _dbus_assert ((address_problem_other == NULL) != (address_problem_type == NULL)); _dbus_assert ((address_problem_type == NULL) == (address_problem_field == NULL)); or be two functions: void _dbus_set_bad_address (DBusError *error, const char *problem); void _dbus_set_bad_address_missing_field (DBusError *error, const char *type, const char *field); I merged most of this. (In reply to comment #30) > Comment on attachment 84510 [details] [review] [review] > [PATCH v2 7/7] Align behavior with document > > Review of attachment 84510 [details] [review] [review]: > ----------------------------------------------------------------- > > There are no callers where address_problem_type and _field don't have the > same NULL'ness, and there shouldn't be. > > Since the intention seems to be that you pass either type-and-field, or just > other, I think this should either have something like: > > _dbus_assert ((address_problem_other == NULL) != (address_problem_type == > NULL)); > _dbus_assert ((address_problem_type == NULL) == (address_problem_field == > NULL)); Yeah, but I think once we do this, we need update the document. > > or be two functions: > > void _dbus_set_bad_address (DBusError *error, const char *problem); > void _dbus_set_bad_address_missing_field (DBusError *error, const char > *type, const char *field); This is seems more complicated. Given that this is an internal API and it's not much better if we change it, I agree to just left as it is or the original patch is acceptable given that all the user of this function are well done. Comment on attachment 84503 [details] [review] [PATCH v2 1/7] Cleanup: polish verbose mode checking applied Comment on attachment 84505 [details] [review] [PATCH v2 3/7] Cleanup: simplify assertion check applied Comment on attachment 84508 [details] [review] [PATCH v2 5/7] Fix comment about atomic operations applied Comment on attachment 84509 [details] [review] [PATCH v2 6/7] Fix debug output about dbus server ref count applied Some fixed in 1.7.x, remainder are WONTFIX. |
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.