Syslinux - May 2019 - [PATCH] (vesa)menu.c32: Add support for BLS

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?
>
> > 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?
>
> <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.
>
> > +        } 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.

Thanks.

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

> > > > 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.
 
 
If I may, as a side-note... 

Let's not confuse "embrace" (and even "push for") with
"support".

Let's not confuse one non-neutral "document" (that someone wrote
as
part of the desires of one organization) with some kind of "universal 
specification that should rather be followed by everyone for the 
improvement of humanity and technology".

I'm not necessarily "absolutely opposed" - I'm not
enthusiastic about
it (either) - but let's be clear about what the document is, and what 
it is not.

Regards,
Ady.