Uses libc's uuid to check that the uuid supplied
at the command line is valid. If not, then bhyve
will exit with an appropriate error message.
Details
Run bhyve passing a valid UUID, for ex:
bhyve -U 7c27ce48-7f96-4c2e-9ab5-3ccdd8a2ef39
and it should display the info message
Run
bhyve -U invalid-uuid
and see bhyve fail with an error message
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 38938 Build 35827: arc lint + arc unit
Event Timeline
Adding Michael as he raised the bug here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=255467
In head this code has changed a bit, and you'd rather want to check at the place that the associated config file is used (so that you'd catch invalid UUID strings from config files).
The uuid is being correctly validated in smbiostbl.c:smbios_type1_initializer() so there's no need to do it again. An fprintf to stderr there indicatiing it is invalid is fine, and the assert on a return from smbios_build() can be changed into an exit(1).
Concur, other than the exit code should be 4, as exit(1) indicates "powered off" and that is not what happened in this case.
Thanks @grehan .
I see that doing the validation twice makes no sense. However I do
like the idea of validating inputs early on in the program execution. It's
also where the rest of the command line argument validation exists, so it seems
like the natural place for it?
So I have taken a slightly different approach to your suggestion, and
replaced guest_uuid_str with the actual guest_uuid.
Thanks for the feedback @jhb ,
you'd rather want to check at the place that the associated config file is used
I am still new to bhyve, and haven't looked at the config file code yet,
but if the config file had an invalid UUID I'd expect a different error message than the
one shown for an invalid command line argument? (from a UX perspective)
I am a pretty new contributor, but wanted to share my opinion - if it's not the
right approach, then I am happy to rework as requested.
usr.sbin/bhyve/smbiostbl.c | ||
---|---|---|
604 ↗ | (On Diff #88472) | This should remain a return (-1); And the assert in bhyverun.c should be changed to an exit(4); |
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
1394 | Hmm, I wonder if we should also exit(4) here as well? and in all the other assert(error == 0) locations? |
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
1394 | Save that for another review :) |
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
1392 | Per style(9) when there is only 1 statement in a block you do not enclose it in { }, so this could become just: if (error) exit(4); |
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
1392 | For the record, style(9) was amended several years ago to allow braces around single-line statements: Space after keywords (if, while, for, return, switch). Two styles of braces (‘{’ and ‘}’) are allowed for single line statements. Either they are used for all single statements, or they are used only where needed for clarity. Usage within a function should be consistent. Forever loops are done with for's, not while's. See https://reviews.freebsd.org/V3 / https://cgit.freebsd.org/src/commit/?id=abb14e5deda3 |
With examples like Apple's "goto fail" bug (https://www.imperialviolet.org/2014/02/22/applebug.html) I'm surprised style(9) only allows, and doesn't recommend or mandate braces around single-line statements.
Style(9) reflects the actual style in the tree. It tries to balance readability and safety. Too many lines with just } on them makes code harder to read. Plus studies on the topic are inconclusive as to bug reduction. Finally, the braces style was inherited from Bell Labs with the 7th edition code, some of which survives to today... still, time marches on and with larger screens an extra brace isn't as big a deal as it was years ago, which is why it's allowed but not mandatory.
Great work James all who have provided review!
Towards the goal of consistency in the error messages, perhaps consider changing "Invalid UUID provided" to simply "Invalid UUID" or "invalid UUID configuration" to follow the example of other error messages? Perhaps the maintainers have opinions on this.
@jhb apologies.. I only just realised this had outstanding feedback :-o
I have made the changes you requested.
usr.sbin/bhyve/bhyverun.c | ||
---|---|---|
1390 |
There's no manpage change needed. It's just adding an error message and always exiting instead of an assert that only exits for a debug build.
It is simple to explain: some older revisions touched some of the monitored path in the herald transcript and triggered manpages. You can use the history compare button on the Phabricator UI to figure that out as well.