Adds uuid validation to bhyve(8) arg parsing

[me_jamesrm.com]

Summary

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.

Test Plan

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

[me_jamesrm.com]

Adding Michael as he raised the bug here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=255467

[me_jamesrm.com]

• Whitespace

[me_jamesrm.com]

Fix history

[me_jamesrm.com]

Style feedback thanks @kp

[jhb]

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).

[bcr]

OK from manpages.

[grehan]

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).

[rgrimes]

In D30050#674660, @grehan wrote:

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.

[me_jamesrm.com]

Review feedback

[me_jamesrm.com]

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.

[me_jamesrm.com]

Ok, after additional feedback from @rgrimes I have reworked the patch. Thanks for the feedback.

@jhb I've looked a little more into the config stuff and had a couple of questions:

1. Where should this UUID be in the config data?
2. Where should it be validated?

[rgrimes]

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);

[me_jamesrm.com]

Feedback

[me_jamesrm.com]

Cleanup

[me_jamesrm.com]

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?

[grehan]

usr.sbin/bhyve/bhyverun.c

1394

Save that for another review :)

[rgrimes]

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);

[me_jamesrm.com]

style(9)

[rpokala]

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

[bcran]

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.

[imp]

In D30050#675075, @bcran wrote:

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.

[editor_callfortesting.org]

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.

[me_jamesrm.com]

Update's error message as per the suggestion from @editor_callfortesting.org

[jhb]

usr.sbin/bhyve/bhyverun.c

1390

usr.sbin/bhyve/smbiostbl.c

602 ↗

(On Diff #88778)

Direct use of fprintf to stderr isn't used in bhyve since a uart on stdio can put the tty into raw mode. There are wrapper macros (EPRINTLN or the like) that are used in bhyve instead.

[me_jamesrm.com]

@jhb apologies.. I only just realised this had outstanding feedback :-o

I have made the changes you requested.

[pauamma_gundo.com]

I don't see any manual page change. Did I miss something?

[jhb]

usr.sbin/bhyve/bhyverun.c

1390

[jhb]

In D30050#792002, @pauamma_gundo.com wrote:

I don't see any manual page change. Did I miss something?

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.

[pauamma_gundo.com]

In D30050#793504, @jhb wrote:

In D30050#792002, @pauamma_gundo.com wrote:

I don't see any manual page change. Did I miss something?

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.

Doesn't explain why the manpages group reviewer.

[khng]

In D30050#793887, @pauamma_gundo.com wrote:

Doesn't explain why the manpages group reviewer.

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.