|
Description
Simon McVittie
2016-10-10 14:36:51 UTC
Created attachment 127281 [details] [review] _dbus_validity_to_error_message: add missing cases From: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: split out from a larger commit] Reviewed-by: Simon McVittie <smcv@debian.org> Created attachment 127282 [details] [review] Move defaults for some switches into a default case This is clearly equivalent, and quiets -Wswitch-default. Based on part of a patch by Thomas Zimmermann. Created attachment 127283 [details] [review] Trivial config parser: enumerate the elements we don't care about From: Thomas Zimmermann <tdz@users.sourceforge.net> This quiets -Wswitch-enum warnings. The trivial config parser is used by the setuid activation helper, and only handles the elements whose contents influence the operation of that helper: system service directories, the setuid activation helper itself, the bus uid, and the bus type. [smcv: split out from a larger commit; add justification; move ELEMENT_SERVICEDIR start handler to a functionally equivalent list of elements whose content we are going to process later] Reviewed-by: Simon McVittie <smcv@debian.org> Created attachment 127284 [details] [review] Stringify DBUS_AUTH_STATE_INVALID From: Thomas Zimmermann <tdz@users.sourceforge.net> [smcv: split out from a larger commit] Reviewed-by: Simon McVittie <smcv@debian.org> Created attachment 127285 [details] [review] Marshalling tests: make integer generation more concise From: Thomas Zimmermann <tdz@users.sourceforge.net> This also avoids -Wswitch-default warnings. [smcv: split out from a larger commit] Reviewed-by: Simon McVittie <smcv@debian.org> Created attachment 127286 [details] [review] Bus driver: add default BusDriverFound switch cases If we get an impossible result, treat it as BUS_DRIVER_FOUND_ERROR. https://github.com/smcv/dbus/tree/switch has my work-in-progress on breaking up and reviewing the original attachments: the reviewed bits attached here, my new commits also attached here, and a large commit for "the rest". Created attachment 129005 [details] [review] WiP: remaining -Wswitch-* fixes from Thomas Zimmermann These need reviewing, and in some cases clarifying. --- I pushed previous attachments to master for 1.11.10. Here is what's left to break up and review. Comment on attachment 129005 [details] [review] WiP: remaining -Wswitch-* fixes from Thomas Zimmermann Review of attachment 129005 [details] [review]: ----------------------------------------------------------------- Looks to me like a lot of the `default` cases should call `_dbus_assert_not_reached()`, since they should never be reached at runtime. ::: bus/dispatch.c @@ +3556,5 @@ > _dbus_warn ("Unexpected message after auto activation"); > goto out; > + > + default: > + break; Would this one be better off having a warning (i.e. same as the GOT_SOMETHING_ELSE case)? @@ +4252,5 @@ > _dbus_warn ("Unexpected message after auto activation"); > goto out; > + > + default: > + break; Would this one be better off having a warning (i.e. same as the GOT_SOMETHING_ELSE case)? (In reply to Philip Withnall from comment #9) > Looks to me like a lot of the `default` cases should call > `_dbus_assert_not_reached()`, since they should never be reached at runtime. Yes. I'll attach more patches soon. Created attachment 130424 [details] [review] [1] bus dispatch tests: treat impossible message_kind as GOT_SOMETHING_ELSE check_got_service_info() can't actually return an invalid GotServiceInfo, but if it somehow does, we want to fail the test. GOT_SOMETHING_ELSE already has that effect, and a similar meaning. Based on a patch from Thomas Zimmermann. Created attachment 130425 [details] [review] [2] dbus-daemon: silence -Wswitch-default There should be no way signal_handler() can be called for a signal we didn't ask for. If it somehow happens, ignore it. Based on a patch from Thomas Zimmermann. Created attachment 130426 [details] [review] [3] _dbus_lm_strerror: move default behaviour inside switch This silences -Wswitch-default. Part of a larger commit from Thomas Zimmermann. Created attachment 130427 [details] [review] [4] dbus-spawn: assert impossible returns from read functions don't happen This silences -Wswitch-default. Based on a patch from Thomas Zimmermann. Created attachment 130428 [details] [review] [5] test, tools: assert impossible values of local enums are not reached Based on part of a patch from Thomas Zimmermann. Created attachment 130429 [details] [review] [6] dbus-monitor: handle default case for binary mode header Also comment why it's OK to not do anything for the modes that don't have a header. We are effectively treating the default case as one of those, on the assumption that future modes are more likely to lack a header than to have one. Based on part of a patch from Thomas Zimmermann. Created attachment 130430 [details] [review] [7] dbus-launch: clarify signal handler We only register signal_handler() for the three signals that we want to handle as "kill dbus-daemon and exit", so there's no point in the switch. Silence -Wswitch-default by removing it altogether. The variable name got_fatal_signal and the verbose message are both misleading, because actually this is a handler for multiple signals, not just SIGHUP. Rename them to be generic. Based on part of a patch from Thomas Zimmermann. Created attachment 130431 [details] [review] [8] DBusTransport: assert that invalid results don't happen This silences -Wswitch-default. Based on part of a patch from Thomas Zimmermann. Created attachment 130432 [details] [review] [9] DBusTransport: be explicit about _dbus_auth_do_work() results This silences -Wswitch-enum. Based on part of a patch from Thomas Zimmermann. Created attachment 130433 [details] [review] [10] _dbus_global_lock: move success case up into switch This silences -Wswitch-default. Based on part of a patch from Thomas Zimmermann. Created attachment 130434 [details] [review] [11] sysdeps: assert that log severity is one we expect This silences -Wswitch-default. Based on part of a patch from Thomas Zimmermann. Created attachment 130435 [details] [review] [12] config-parser: treat impossible policy type as IGNORED This silences -Wswitch-default. Based on part of a patch from Thomas Zimmermann. Created attachment 130436 [details] [review] [13] config-parser: assert elements are of a known type This silences -Wswitch-default. Based on part of a patch from Thomas Zimmermann. Created attachment 130437 [details] [review] [14] config-parser tests: explicitly skip non-comparable elements For these types, the tagged union in the Element struct does not store anything we could usefuly compare. Based on part of a patch from Thomas Zimmermann. Created attachment 130438 [details] [review] [15] bus policy: assert that no invalid rule types are seen This silences -Wswitch-default. Based on part of a patch from Thomas Zimmermann. Created attachment 130439 [details] [review] [16] Stop opting out of -Wswitch-enum and -Wswitch-default Created attachment 130442 [details] [review] [5] test, tools: assert impossible values of local enums are not reached Based on part of a patch from Thomas Zimmermann. --- Fix another instance of -Wswitch-enum when building for mingw with tests enabled. Comment on attachment 130424 [details] [review] [1] bus dispatch tests: treat impossible message_kind as GOT_SOMETHING_ELSE Review of attachment 130424 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130425 [details] [review] [2] dbus-daemon: silence -Wswitch-default Review of attachment 130425 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130426 [details] [review] [3] _dbus_lm_strerror: move default behaviour inside switch Review of attachment 130426 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130427 [details] [review] [4] dbus-spawn: assert impossible returns from read functions don't happen Review of attachment 130427 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130428 [details] [review] [5] test, tools: assert impossible values of local enums are not reached Review of attachment 130428 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130429 [details] [review] [6] dbus-monitor: handle default case for binary mode header Review of attachment 130429 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130430 [details] [review] [7] dbus-launch: clarify signal handler Review of attachment 130430 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130431 [details] [review] [8] DBusTransport: assert that invalid results don't happen Review of attachment 130431 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130432 [details] [review] [9] DBusTransport: be explicit about _dbus_auth_do_work() results Review of attachment 130432 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-transport.c @@ +752,5 @@ > + case DBUS_AUTH_STATE_WAITING_FOR_INPUT: > + case DBUS_AUTH_STATE_WAITING_FOR_MEMORY: > + case DBUS_AUTH_STATE_HAVE_BYTES_TO_SEND: > + case DBUS_AUTH_STATE_NEED_DISCONNECT: > + case DBUS_AUTH_STATE_INVALID: Do we not want to handle DBUS_AUTH_STATE_INVALID separately? Comment on attachment 130433 [details] [review] [10] _dbus_global_lock: move success case up into switch Review of attachment 130433 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130434 [details] [review] [11] sysdeps: assert that log severity is one we expect Review of attachment 130434 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130435 [details] [review] [12] config-parser: treat impossible policy type as IGNORED Review of attachment 130435 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130436 [details] [review] [13] config-parser: assert elements are of a known type Review of attachment 130436 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130437 [details] [review] [14] config-parser tests: explicitly skip non-comparable elements Review of attachment 130437 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130438 [details] [review] [15] bus policy: assert that no invalid rule types are seen Review of attachment 130438 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 130439 [details] [review] [16] Stop opting out of -Wswitch-enum and -Wswitch-default Review of attachment 130439 [details] [review]: ----------------------------------------------------------------- yay, ++ Comment on attachment 130442 [details] [review] [5] test, tools: assert impossible values of local enums are not reached Review of attachment 130442 [details] [review]: ----------------------------------------------------------------- ++ Created attachment 130743 [details] [review] DBusTransport: be explicit about _dbus_auth_do_work() results Replacement for Attachment #130432 [details], addressing Philip's review comments. I applied the rest, except for Attachment #130439 [details] which needs to be last. Comment on attachment 130743 [details] [review] DBusTransport: be explicit about _dbus_auth_do_work() results Review of attachment 130743 [details] [review]: ----------------------------------------------------------------- r+ Thanks, fixed in git for 1.11.12 |
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.