Re: [PATCH 1/2] Add support for extlinux.conf

[Alexander Shiyan <eagle.alexander923@gmail.com>]

Hello Ahmad.

> Thanks for your patch!
>
> On 4/16/26 3:45 PM, Alexander Shiyan wrote:
> > This adds support for the extlinux.conf configuration format, commonly
> > used by Syslinux and many Linux distributions. The configuration file
> > is typically located at /boot/extlinux/extlinux.conf or
> > /extlinux/extlinux.conf and defines boot entries with kernel, initrd,
> > device tree, and command line options.
...
> > +static int extlinux_boot(struct bootentry *be, int verbose, int dryrun)
> > +{
> > +     struct extlinux_entry *e =
> > +             container_of(be, struct extlinux_entry, entry);
> > +     char *kernel_abs, *initrd_abs = NULL, *fdt_abs = NULL;
> > +     struct bootm_data data = {};
> > +     int ret;
> > +
> > +     bootm_data_init_defaults(&data);
> > +
> > +     data.dryrun = max_t(int, dryrun, data.dryrun);
> > +     data.verbose = max(verbose, data.verbose);
> > +     data.appendroot = true;
>
> Not sure this is a good idea because of potential of clashing with
> extlinux appended command line options.

globalvar_add_bool("extlinux.fix_root", NULL); ?

...
> > +     if (e->append)
> > +             globalvar_add_simple("linux.bootargs.dyn.extlinux", e->append);
> > +
> > +     pr_info("Booting extlinux label '%s'\n", e->label);
> > +
> > +     ret = bootm_boot(&data);
>
> Please make use of new bootm_entry instead, so it can be used with devboot:
>
> https://github.com/barebox/barebox/blob/next/Documentation/user/devboot.rst

I haven't figured out how to do this yet...

...
> > +static struct extlinux_entry *parse_extlinux_conf(const char *abspath,
> > +                                               const char *rootpath)
> > +{
> > +     char *buf, *bufptr, *line, *p, *default_label = NULL;
> > +     struct extlinux_entry *entry = NULL;
> > +
> > +     bufptr = read_file(abspath, NULL);
> > +     if (!bufptr)
> > +             return ERR_PTR(-errno);
> > +
> > +     for (p = bufptr; *p; p++)
> > +             if (*p == '\r')
> > +                     *p = '\n';
>
> I think this can be dropped.
>
> > +
> > +     buf = bufptr;
> > +     while ((line = strsep(&buf, "\n")) != NULL) {
>
> And then you replace the set searched here with "\r\n"

The end of the line may be \n, \r, \n\r.
We are simply converting it to a uniform format.

> > +             char *key, *val;
> > +
> > +             line = skip_spaces(line);
> > +
> > +             if (*line == '#' || *line == '\0')
> > +                     continue;
> > +
>
> Nitpick: I think below can be simplified to:
>
> > +             key = line;
> > +             val = strchr(line, ' ');
> > +             if (!val)
> > +                     val = strchr(line, '\t');
> > +             if (val) {
> > +                     *val++ = '\0';
> > +                     val = skip_spaces(val);
> > +             } else
> > +                     continue;
>
> key = strsep(&line, ' \t');
> val = strsep(&line, '\n');
> val = isempty(val) ? NULL : skip_spaces(val);

strsep inserts a null byte in place of the separator; this will
corrupt the buffer,
and consequently, we will not be able to find the val.

> > +
> > +             if (!default_label) {
> > +                     if (!strcasecmp(key, "DEFAULT"))
> > +                             default_label = xstrdup(val);
> > +
> > +                     continue;
> > +             }
> > +
> > +             if (!strcasecmp(key, "LABEL")) {
> > +                     if (!strcmp(val, default_label)) {
>
> defaut_label might not have been set here?

No: if (!default_label) ... continue;