Re: CVS commit: src/sys

[Paul Goyette <paul%whooppee.com@localhost>]

On Thu, 8 May 2025, Taylor R Campbell wrote:

> > Module Name:    src
> > Committed By:   pgoyette
> > Date:           Tue May  6 18:16:12 UTC 2025
> >
> > Modified 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/59207
>
> The 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 
rvp%sdf.org@localhost - 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 | paul%whooppee.com@localhost    |
| Software Developer  | 6E2E 05FD 15CE 9F2D 5102 | pgoyette%netbsd.org@localhost  |
| & Network Engineer  |                          | pgoyette99%gmail.com@localhost |
+---------------------+--------------------------+----------------------+