+++ This bug was initially created as a clone of Bug #97357 +++ The patches on Bug #97357 include some for -Wswitch-enum and -Wswitch-default (Attachment #127058 [details], Attachment #127059 [details]). To make this more reviewable, I would prefer to fix these two warnings as a unit, but with each commit doing one thing, by following this process: * Land what's on Bug #97357, or use my "warnings" branch on Github as a base <https://github.com/smcv/dbus/tree/warnings> * Enable the warnings locally but do not make them fatal, by building with make CFLAGS="-Wswitch-enum -Wswitch-default -Wno-error=switch-enum -Wno-error=switch-default" * Go through each affected switch or closely related group of switches (for example switching over values of the same enum can probably be grouped), fix both warnings at the same time, and do one commit per group * After all those commits, add one commit removing -Wno-switch-enum and -Wno-switch-default from the AX_COMPILER_FLAGS* arguments so that this doesn't regress in future I hope that makes sense.
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.