Page MenuHomeFreeBSD

Create three helper functions for parsing boot args.
ClosedPublic

Authored by imp on Jul 10 2018, 5:27 AM.
Tags
None
Referenced Files
F105863178: D16205.id45114.diff
Sat, Dec 21, 6:22 PM
Unknown Object (File)
Wed, Nov 27, 12:16 AM
Unknown Object (File)
Nov 19 2024, 6:27 PM
Unknown Object (File)
Oct 30 2024, 3:25 AM
Unknown Object (File)
Oct 24 2024, 5:10 PM
Unknown Object (File)
Oct 9 2024, 8:52 PM
Unknown Object (File)
Sep 27 2024, 9:33 AM
Unknown Object (File)
Sep 17 2024, 8:42 PM
Subscribers

Details

Summary

boot_parse_arg to parse a single arg
boot_parse_cmdline to parse a command line string
boot_parse_args to parse all the args in a vector

All these routines return an int that's the bitmask of the args
translated to RB_* flags. As a special case, the 'S' flag sets the
comconsole_speed env var. Any arg that looks like a=b will set the env
key 'a' to value 'b'. If =b is omitted, 'a' is set to '1'. This
should help us reduce the number of redundant copies of these routines
in the tree. It should also give a more uniform experience between
platforms.

Also, invent a new flag RB_PROBE that's set when 'P' is parsed. On
x86 + BIOS, this means 'probe for the keyboard, and if it's not there
set both RB_MULTIPLE and RB_SERIAL (which means show the output on
both video and serial consoles, but make serial primary). Others it
may be some similar concept of probing, but it's loader dependent
what, exactly, it means.

These routiens are suitable for /boot/loader and/or the kernel,
though they may not be suitable for the tightly hand-rolled-for-space
environments like boot2.

Use boot_parse_* to parse command line args and retire cut-n-paste code that was substantially identical.

Test Plan

Note: this is just for the in-kernel copies. There's other code that does the boot loader stuff that I need to look at. I doubt we'd be able to do much with the 'boot' files since they are space constrained. I may need to not define so many copies of howto_names...

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp added reviewers: jhb, kevans, ian, adrian, bz.
manu added inline comments.
sys/dev/ofw/ofw_subr.c
203 ↗(On Diff #45101)

you |= everywhere except here, is that indented ?

sys/mips/ingenic/jz4780_machdep.c
250 ↗(On Diff #45101)

This should be boot_parse_args, no?

sys/sys/boot.h
110 ↗(On Diff #45101)

Here and down in boot_parse_args, boot_parse_arg is passed a (non-existent?) has_kbd as a second parameter (that doesn't exist)?

116 ↗(On Diff #45101)

s/parsee/parse

sys/dev/ofw/ofw_subr.c
203 ↗(On Diff #45101)

Many other places, we need |= because we build the args up. Here we don't, but it would be good for the hobgoblins of my mind...

sys/mips/ingenic/jz4780_machdep.c
250 ↗(On Diff #45101)

yes. Too many platforms...

sys/sys/boot.h
110 ↗(On Diff #45101)

Doh! that's what I get for making a last-minute decision to eliminate it :(

Review nits noted so far.

imp marked 5 inline comments as done.Jul 10 2018, 3:12 PM

Seems reasonable...hurray for de-duplication.

This revision is now accepted and ready to land.Jul 10 2018, 4:23 PM

Rework somewhat before sys/boot.h gets too out of control.

  • Eliminate 3 more copies of boot arg parsing.
  • Transition to boot_env_to_howto and boot_howto_to_env in the boot
This revision now requires review to proceed.Jul 11 2018, 2:07 AM

The observant will notice there's still a copy in the EFI loader... that will be removed in a different review

sys/kern/subr_boot.c
98–116 ↗(On Diff #45132)

I stole this from the efi loader which stole it from boot2 I think. It's not quite right to drop in, but it's closeish.

sys/x86/xen/pv.c
289 ↗(On Diff #45132)

are the args passed in from XEN hypervisor really a comma separated list?

I wonder if we could use boot_parse_cmdline() here instead...

It's also odd that very similar code is in the arm uboot interface file, which is why I ask... One of them is a copy that likely doesn't work quite right in that environment, and I suspect it's the arm one...

sys/kern/subr_boot.c
116 ↗(On Diff #45132)

It appears as if most of this file has been stolen from various places, yet no attribution to the original authors has been done.

sys/sys/boot.h
4 ↗(On Diff #45132)

5 lines does not meet the 25% content

sys/sys/boot.h
4 ↗(On Diff #45132)

Umm, it's now 5 lines of 5 real lines. that's 100% change. Totally qualifies for a copyright claim. I should remove Roger's, but didn't.

sys/sys/boot.h
4 ↗(On Diff #45132)

Doh, I scrolled the wrong direction and say 369 line fine.. nvm

sys/sys/boot.h
4 ↗(On Diff #45132)

It does raise the question, though, of where, exactly the old code in subr_boot.c originated. I've but tweaked it.

royger requested changes to this revision.Jul 11 2018, 8:54 AM

LGTM, I think with a small change to boot_parse_cmdline we could remove xen_pv_set_env.

sys/kern/subr_boot.c
52 ↗(On Diff #45132)

unsigned int would be better here IMO.

sys/x86/xen/pv.c
289 ↗(On Diff #45132)

Yes, it's been comma separated so far, using "\n" could be complicated because the xen config file is like:

cmdline="boot_verbose=YES,boot_..."

Do you think you could change boot_parse_cmdline so that the delimiter can be provided as a parameter? Then you could remove xen_pv_set_env and just use boot_parse_cmdline plus boot_env_to_howto.

This revision now requires changes to proceed.Jul 11 2018, 8:54 AM

Given this is multiple architectures, how are you dealing with colliding/different values? Hmm I see you don't touch that part.
How are you dealing with architecture specific ones (not #defined but now part of the switch statement)?
I guess what I am saying is: if we clean this up, can we also break everything and cleanup the #defines along somehow to only have one set of those as well?

That would mean that people have to update loader and kernel together I guess which could be a bit of a hiccup... hmm..

In D16205#344062, @bz wrote:

Given this is multiple architectures, how are you dealing with colliding/different values? Hmm I see you don't touch that part.

There are no such values. RB_* are 100% the same everywhere.

How are you dealing with architecture specific ones (not #defined but now part of the switch statement)?

I've already dealt with that: by defining RB_PROBE and parsing 'S' and 'P' always. It doesn't hurt, and may help.

I guess what I am saying is: if we clean this up, can we also break everything and cleanup the #defines along somehow to only have one set of those as well?

Where are the multiple sets of #defines? I found no evidence of them.

That would mean that people have to update loader and kernel together I guess which could be a bit of a hiccup... hmm..

Since they are already universal, no such hickup is needed.

sys/kern/subr_boot.c
52 ↗(On Diff #45132)

You'd think that, but boothowto is an int and I'm not up for changing that because there's lots of other places there's a hassle...

116 ↗(On Diff #45132)

Yes, I noticed that last night. However, finding the *ORIGINAL* authors is hard because it's been copied over and over and over and over again. It's not at all clear where the original started from. But I'll do a dig and try to find that out as best I can for the bits that were done more recently because in the couple cases I checked, it wasn't the author at the top of the file...

sys/x86/xen/pv.c
289 ↗(On Diff #45132)

Easy enough. done.

In D16205#344159, @imp wrote:
In D16205#344062, @bz wrote:

Given this is multiple architectures, how are you dealing with colliding/different values? Hmm I see you don't touch that part.

There are no such values. RB_* are 100% the same everywhere.

...

Sorry, good morning from here; I was thinking of MODINFO_* ; the other bits.. ignore my noise.

Track down copyright holders / original authors as best I can.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 13 2018, 4:43 PM
This revision was automatically updated to reflect the committed changes.