Details
- Reviewers
scottl brooks wma emaste jhb allanjude trasz rpokala loos marius smh • ian - Commits
- rS294769: Allow new lines as white space for arguments that are parsed to allow
rS294768: Read in /boot/config and /boot.config, like all the other boot
rS294766: RBX_ defines are in rbx.h, move it there.
rS294767: Parse the command line arguments, and do it before we initialize the
rS294765: Move all the separate copies of the same strings into paths.h. There's
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2243 Build 2252: arc lint + arc unit
Event Timeline
Why are efi/boot1/boot1.c and zfsboot.c including libs.h rather than paths.h? For that matter, where is libs.h? I don't see it in the source tree, or referenced anywhere (so it's not generated either).
libs.h -> paths.h
Updating D5038: Move all the separate copies of the same strings into paths.h. There's
nothing machine specific about these.
It always bothered me that those were dup'ed everywhere. Thanks for cleaning this up.
Add RBX_ defines to the mix, they will be separate commits.
Updating D5038: Move all the separate copies of the same strings into paths.h. There's
nothing machine specific about these.
Is it alright to use the RBX_MASK from rbx.h for everything? The various "boot2.c"s currently have different values for that.
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 34 | You're including paths.h for PATH_LOADER then overriding it and no other PATH_XXX entries are used, which seems wasteful, Is there a reason for this? | |
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 34 | That does seem odd. Perhaps we should have both PATH_LOADER and PATH_LOADER_EFI defines, etc. | |
- Move all the separate copies of the same strings into paths.h. There's
- RBX_ defines are in rbx.h, move it there.
- Parse the command line arguments, and do it before we initialize the
- Read in /boot/config and /boot.config, like all the other boot
Updating D5038: Move all the separate copies of the same strings into paths.h. There's
nothing machine specific about these.
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 34 | All paths come from paths.h, but perhaps ed is right. I'll think about that aspect. | |
They do? Which ones are different? All the x86 ones should be the same (and it is a bug if they aren't). The arm ones should likely be the same, but I haven't tested that stuff since the gear they were written for is a bit old. They all look the same to me, and were all originally copied from the x86 stuff. Is there a difference my eyeballs missed?
Ideally the path / rbx cleanup and the addition of new features such as the boot config support for EFI should be committed separately.
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 113 | Check and report other other unexpected errors? | |
| 116 | We shouldn't use Malloc directly IMO, is there for wiring up malloc. IMO AllocatePool FreePool should be use in the EFI code. | |
| 122 | Would be nice to print this in the FreeBSD EFI boot block below, so it keeps all the useful info in one place and well formatted. | |
| 125 | If you have a config above then buf won't be NULL and bufsize will set. IIRC this means that call could fail with EFI_BUFFER_TOO_SMALL. Easiest fix is to free the buffer used to load the config, resets buf to NULL and let load allocate the correct size buffer. | |
sys/boot/arm/at91/boot2/boot2.c
ASKNAME | SINGLE | DFLTROOT | VERBOSE | GDB
sys/boot/arm/ixp425/boot2/boot2.c
ASKNAME | SINGLE | DFLTROOT | VERBOSE | GDB
sys/boot/i386/boot2/boot2.c
ASKNAME | SINGLE | DFLTROOT | KDB | CONFIG | VERBOSE | SERIAL | CDROM | GDB | MUTE | PAUSE | DUAL
sys/boot/mips/beri/boot2/boot2.c
ASKNAME | SINGLE | DFLTROOT | KDB | CONFIG | VERBOSE | SERIAL | CDROM | GDB | MUTE | PAUSE | DUAL
sys/boot/pc98/boot2/boot2.c
ASKNAME | SINGLE | DFLTROOT | KDB | CONFIG | VERBOSE | SERIAL | CDROM | GDB | MUTE | PAUSE | DUAL| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 34 | FWIW, I agree with @emaste - just add PATH_LOADER_EFI to paths.h | |
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 113 | Nope. Those don't matter. We only get EFI_NOT_FOUND when DOTCONFIG doesn't exist, but there was a filesystem. All the other errors don't matter. We'll catch them later when we go to load the kernel. | |
| 116 | Those APIs are a pain in the ass to use. There's no reason to make our life harder than it needs to be, and using those APIs when we have easier ones is just cutting our nose off to spite our face. | |
| 122 | Good point. I'll move it. | |
| 125 | No. We always pass NULL the first time to the read that's buried inside of load, and then we always allocate buf. It was uninitialized before, and so its value doesn't matter. But you are right we should free the buffer. that was an oversight on my part. freeing buf won't reset it to NULL, and there's no requirement for that in the load() code. | |
Updating based on reviews.
- Move all the separate copies of the same strings into paths.h. There's
- RBX_ defines are in rbx.h, move it there.
- Parse the command line arguments, and do it before we initialize the
- Read in /boot/config and /boot.config, like all the other boot
Updating D5038: Move all the separate copies of the same strings into paths.h. There's
nothing machine specific about these.
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 113 | I was thinking about the errors from allocate etc, which are fatal, I guess they should be replicated below, so we'll catch them there. It should also be noted that this may load PATH_DOTCONFIG / PATH_CONFIG, from a different device than that used for PATH_LOADER_EFI, not sure this is the desired behaviour or if it could cause unexpected issues? If so we may need to use try_load, which will need exporting, as its device specific. | |
| 122 | Should we then move it to just before the StartImage call so if we fail in between we don't output it? | |
| sys/boot/efi/boot1/boot1.c | ||
|---|---|---|
| 113 | Well, yes and no. I don't think it is a big deal. It's only going to be an issue if we load /boot/loader, and it exits before calling exit boot services. Fairly theoretical. Fortunately, it is easy to fix. | |
| 122 | Yea. Another highly theoretical case. If there's a UFS filesystem, that has a and /boot.config or /boot/config and no loader.efi and the system also has a ZPOOL, then we'll print the wrong boot args to the EFI console before it boots off ZPOOL, possibly with a second set of arguments that it prints that might be confusing... Again, fairly easy to fix. | |
More updates from the code review.
- Move all the separate copies of the same strings into paths.h. There's
- RBX_ defines are in rbx.h, move it there.
- Parse the command line arguments, and do it before we initialize the
- Read in /boot/config and /boot.config, like all the other boot
- Allow new lines as white space for arguments that are parsed to allow
Updating D5038: Move all the separate copies of the same strings into paths.h. There's
nothing machine specific about these.
Note, I've pushed this change, but if there's other issues, I'll start a new review for them.