[syslinux] [PATCH] (vesa)menu.c32: Add support for BLS

[Sebastian Herbszt]

Gregory Bartholomew wrote:
> On Sat, May 25, 2019 at 3:42 PM Sebastian Herbszt <herbszt at gmx.de>
> wrote:
> >
> > Gregory Lee Bartholomew wrote:
> > > Modern distributions are moving toward a common boot scheme called
> > > "The Boot Loader Specification".
> >
> > Which distributions are using this yet?

[copy & paste from other reply]

> Fedora 30

Not that many for a specification written about 6 years ago.

> >
> > > This patch enables syslinux's
> > > (vesa)menu.c32 modules to parse the drop-in files that are
> > > defined by this new specification.
> >
> > Any reason why you don't try to implement this in syslinux core?

[copy & paste from other reply]

> Not sure what you mean by core, but I was trying to be minimally
> intrusive to the existing code.

If you implement it in the menu system (com32/menu) than only its users
will gain support. If put into ldlinux (com32/elflink/ldlinux) it
should be globally available. A config option like "BLS" could enable
support for it. I guess "$BOOT/loader/entries/" is fixed as
"/loader/entries" for syslinux/extlinux/isolinux. It could depend
on "pxelinux.pathprefix" for pxelinux.

> >
> > <snip>
> >
> > > +static int parse_bls1_file(struct blsdata *bd, const char
> > > *filename) +{
> > > +    FILE *f = NULL;
> > > +    char line[MAX_LINE], *p;
> > > +    char *sort_field;
> > > +
> > > +    dprintf("Opening bls entry: %s ", filename);
> > > +
> > > +    f = fopen(filename, "r");
> > > +    dprintf("%s\n", f ? "ok" : "failed");
> > > +
> > > +    if (!f)
> > > +        return -1;
> > > +
> > > +    refstr_put(bd->filename);
> > > +    bd->filename = refstrdup(filename);
> > > +
> > > +    while (fgets(line, sizeof line, f)) {
> > > +        p = strchr(line, '\r');
> > > +        if (p)
> > > +            *p = '\0';
> > > +        p = strchr(line, '\n');
> > > +        if (p)
> > > +            *p = '\0';
> > > +
> > > +        p = skipspace(line);
> > > +
> > > +        if (looking_at(p, "title")) {
> > > +            refstr_put(bd->title);
> > > +            bd->title = refstrdup(skipspace(p + 5));
> > > +        } else if (looking_at(p, "version")) {
> > > +            refstr_put(bd->version);
> > > +            bd->version = refstrdup(skipspace(p + 7));
> > > +            bd->version0 = padver(bd->version);
> > > +        } else if (looking_at(p, "machine-id")) {
> > > +            refstr_put(bd->machine_id);
> > > +            bd->machine_id = refstrdup(skipspace(p + 10));
> > > +        } else if (looking_at(p, "linux")) {
> > > +            refstr_put(bd->linux_);
> > > +            bd->linux_ = refstrdup(skipspace(p + 5));
> > > +        } else if (looking_at(p, "initrd")) {
> > > +            refstr_put(bd->initrd);
> > > +            bd->initrd = refstrdup(skipspace(p + 6));
> >
> > According to the specification "initrd" may appear more than once.
>
> Sorry, I missed a few of your questions in my earlier response.
>
> Ah, I missed that. I guess I'll have to take a look at how syslinux
> handles multiple initrd's and do something similar.

Try to put a comma separated list of all initrd files into bd->initrd.
I think core should handle it for KT_LINUX.

> >
> > > +        } else if (looking_at(p, "efi")) {
> > > +            refstr_put(bd->efi);
> > > +            bd->efi = refstrdup(skipspace(p + 3));
> > > +        } else if (looking_at(p, "options")) {
> > > +            /* The "options" keyword can be specified multiple
> > > times */
> > > +            int len = bd->options ? strlen(bd->options) : 0;
> > > +            int xlen;
> > > +            p = skipspace(p + 7);
> > > +            xlen = strlen(p);
> > > +            if (len && xlen) {
> > > +                /* Grab one space char from the file */
> > > +                p--;
> > > +                xlen++;
> > > +            }
> > > +            bd->options = realloc(bd->options, len + xlen + 1);
> > > +            memcpy(bd->options + len, p, xlen + 1);
> > > +        } else if (looking_at(p, "devicetree")) {
> > > +            refstr_put(bd->devicetree);
> > > +            bd->devicetree = refstrdup(skipspace(p + 10));
> > > +        } else if (looking_at(p, "architecture")) {
> > > +            refstr_put(bd->architecture);
> > > +            bd->architecture = refstrdup(skipspace(p + 12));
> > > +        }
> > > +    }
> > > +
> > > +    fclose(f);
> > > +
> > > +    p = get_word(bls_sort_by, &sort_field);
> > > +    while (sort_field && *sort_field != '\0') {
> > > +        const char *sf = NULL;
> > > +
> > > +        if (looking_at(sort_field, "title")) {
> > > +            sf = bd->title;
> > > +        } else if (looking_at(sort_field, "version")) {
> > > +            sf = bd->version0;
> > > +        } else if (looking_at(sort_field, "machine-id")) {
> > > +            sf = bd->machine_id;
> > > +        }
> > > +
> > > +        if (sf) {
> > > +            bd->sort_field = realloc(
> > > +                bd->sort_field,
> > > +                strlen(bd->sort_field) + strlen(sf) + 1
> > > +            );
> > > +            strcat(bd->sort_field, sf);
> > > +        }
> > > +        p = get_word(skipspace(p), &sort_field);
> > > +    }
> > > +
> > > +    return (bd->linux_ || bd->efi) ? 0 : -1;
> > > +}
> >
> > <snip>
> >
> > > +static int parse_bls1_dir(const char *dirname)
> > > +{
> > > +    DIR *d = NULL;
> > > +    char *filename = NULL;
> > > +    struct dirent *de = NULL;
> > > +    struct blsdata *nbd = NULL, **bdx = NULL;
> > > +    int i, n_bdx = 0, n_bd = 0;
> > > +    int rv = -1, fn_len, dn_len;
> > > +    struct menu *m = current_menu;
> > > +
> > > +    if (!*dirname)
> > > +        return -1;
> > > +
> > > +    dprintf("Opening bls entries directory %s ", dirname);
> > > +
> > > +    d = opendir(dirname);
> > > +    dprintf("%s\n", d ? "ok" : "failed");
> > > +
> > > +    if (!d)
> > > +        return -1;
> > > +
> > > +    dn_len = strlen(dirname);
> > > +
> > > +    /* Process up to 128 files */
> > > +    /* https://wiki.syslinux.org/wiki/
> > > +       index.php?title=Syslinux_1_Changelog#Changes_in_1.37 */
> > > +    while ((de = readdir(d)) != NULL && n_bd < 128) {
> >
> > Are you sure this limit is still accurate?
>
> No, I'm not. It seemed like a reasonable limit though. If the vote is
> that there should not be a limit, I'm cool with taking it out. The
> only other place in the code that I wrote that might be impacted is
> the auto-numbering for the labels. I reserved only three digits for
> the auto-numbered label names -- BLS000 ... BLS127. I'm not sure if
> that is the best way to handle the labels anyway. If anyone has
> suggestions, I'm all ears.

I don't think a limit is required. It is unlikely people start
creating lots of config snippets.
The auto-numbered-labels aren't that bad. The title is not unique
and even a combination of title and version doesn't have to be.
By the way, you might want to add version to menulabel if available.

>
> Thanks.

Sebastian