On Thu, 8 May 2025, Taylor R Campbell wrote:
Module Name: src Committed By: pgoyette Date: Tue May 6 18:16:12 UTC 2025Modified Files: src/sys/arch/i386/stand/boot: boot2.c src/sys/arch/i386/stand/lib: bootmenu.c libi386.h src/sys/arch/i386/stand/pxeboot: main.c src/sys/lib/libsa: bootcfg.c bootcfg.h Log Message: Allow the dev= command when processing /boot.cfg file. This addresses kern/59207The goal of this change looks good, but I worry that: (a) this is a high-risk change because if it breaks someone's bootloader then it renders the machine nonbootable (and we have very limited automatic testing of bootloaders -- which is a problem in itself, not your fault!), and (b) I'm confused by how some parts of it are relevant to the goal, particularly these hunks: --- a/sys/lib/libsa/bootcfg.c Tue May 06 17:12:33 2025 +0000 +++ b/sys/lib/libsa/bootcfg.c Tue May 06 18:16:12 2025 +0000 ... @@ -227,8 +227,6 @@ perform_bootcfg(const char *conf, bootcf bootcfg_info.consdev = value; } else if (!strncmp(key, "root", 4)) { bootcfg_info.root = value; - } else if (!strncmp(key, BOOTCFG_CMD_LOAD, 4)) { - command(BOOTCFG_CMD_LOAD, value); } else if (!strncmp(key, "format", 6)) { printf("value:%c\n", *value); switch (*value) { @@ -251,8 +249,6 @@ perform_bootcfg(const char *conf, bootcf } } else if (!strncmp(key, "clear", 5)) { bootcfg_info.clear = !!atoi(value); - } else if (!strncmp(key, BOOTCFG_CMD_USERCONF, 8)) { - command(BOOTCFG_CMD_USERCONF, value); } else { command(key, value); } Were these branches already dead code? Are they _now_ dead code on x86, but possibly would have been still live on non-x86 bootloaders?
As seen in the PR audit trail, the patch here was provided bby r...@sdf.org - perhaps s/he can address these two hunks?
Can you expand on how this change works and what you did to test it?
Testing was simple. I confirmed that an interactive dev command still worked (by observing the saved default value), and that the command now also works when embedded in boot.cfg file. I've also been using the fix on my local machine with no ill effects.
As a general rule, I think we should post bootloader changes for review to tech-kern@ and relevant port-*@ before committing, because they are so high-risk. It would also be good to state in the commit message what testing has been done.
Sorry. I did post to the PR audit trail a request for comments, but nothing wsas received. +---------------------+--------------------------+----------------------+ | Paul Goyette (.sig) | PGP Key fingerprint: | E-mail addresses: | | (Retired) | 1B11 1849 721C 56C8 F63A | p...@whooppee.com | | Software Developer | 6E2E 05FD 15CE 9F2D 5102 | pgoye...@netbsd.org | | & Network Engineer | | pgoyett...@gmail.com | +---------------------+--------------------------+----------------------+
