[PATCH v2 4/6] net: eth_bootdev_hunt() must not try to boot

[Simon Glass]

Hi Heinrich,

On Wed, 27 Nov 2024 at 06:43, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 27.11.24 14:08, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 26 Nov 2024 at 09:16, Heinrich Schuchardt
> > <heinrich.schuchardt at canonical.com> wrote:
> >>
> >> On 26.11.24 16:38, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Tue, 26 Nov 2024 at 00:42, Heinrich Schuchardt
> >>> <heinrich.schuchardt at canonical.com> wrote:
> >>>>
> >>>> On 11/26/24 01:32, Simon Glass wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Sat, 23 Nov 2024 at 14:46, Heinrich Schuchardt
> >>>>> <heinrich.schuchardt at canonical.com> wrote:
> >>>>>>
> >>>>>> eth_bootdev_hunt() should not execute dhcp_run() as this itself would load
> >>>>>> a file and boot it if autostart=yes.
> >>>>>>
> >>>>>> Instead just check that there is a network device.
> >>>>>
> >>>>> If you look at dhcp_run() you will see an 'autoload' parameter. This
> >>>>> is 'false' in this code, so it never loads a file.
> >>>>
> >>>> This is not the complete truth.
> >>>>
> >>>> If $autostart=true and CONFIG_NET=y, dhcp_run(false) still tries to load
> >>>> a file.
> >>>
> >>> Here is the code:
> >>>
> >>> int dhcp_run(ulong addr, const char *fname, bool autoload)
> >>> {
> >>> char *dhcp_argv[] = {"dhcp", NULL, (char *)fname, NULL};
> >>> struct cmd_tbl cmdtp = {}; /* dummy */
> >>> char file_addr[17];
> >>> int old_autoload;
> >>> int ret, result;
> >>>
> >>> log_debug("addr=%lx, fname=%s, autoload=%d\n", addr, fname, autoload);
> >>> old_autoload = env_get_yesno("autoload");
> >>> ret = env_set("autoload", autoload ? "y" : "n");
> >>> if (ret)
> >>> return log_msg_ret("en1", -EINVAL);
> >>>
> >>> It respects the autoload parameter. If there is a bug in that, we
> >>> should fix it, but it seems to work OK to me. Please fix your commit
> >>> message.
> >>
> >> The cited code only sets "autoload". It does not adjust "autostart".
> >>
> >> As I have shown below this leads to trying to boot via DHCP.
> >>
> >> This is exactly what the commit message says: "load a file and boot it
> >> if autostart=yes".
> >
> > Oh, I just thought they were the same and I didn't remember about autostart
> >
> >>
> >>>
> >>>>
> >>>> In all cases dhcp_run() wastes boot time.
> >>>
> >>> Yes, it uses time. With bootstd the hunters are only called as needed.
> >>> In your case you are calling all of them at once.
> >>>
> >>> Is there a way to just call hunters which relate to block devices, perhaps?
> >>
> >> We need to probe network to provide the simple network protocol
> >> (lib/efi_loader/efi_net.c) but we don't need to execute DHCP.
> >>
> >> We also need the correct device-tree provided by the extension hunter.
> >
> > It would be good to get some use of that, as it has no tests really.
> >
> > As I ask on the other patch, why do you need to probe?
>
> You asked on the Network protocol.
>
> If your idea is that the concept of extensions is not yet deployed, is
> there a good way to exclude BOOTDEV_HUNTER(extension_bootdev_hunter)?

Well, my approach with bootstd was to try to reverse-engineer what
distro_boot did and extensions was one of those things.

I am not suggesting excluding it, merely that I am pleased that it is
being used!

[..]

Regards,
Simon