discover/grub: Allow to set a default index for BLS entries

When the BLS support was added, the conclusion was that default indexes
didn't apply for BLS snippets. But for GRUB 2 the indexes refers to the
boot menu entries in memory, regardless of how these were generated.

Since in GRUB 2 is valid to set a default index even for menu entries
generated from BLS fragments, allow this to also be done in Petitboot.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

---

 discover/grub2/blscfg.c                       | 17 +++++++++---
 test/parser/Makefile.am                       |  1 +
 test/parser/test-grub2-blscfg-default-index.c | 38 +++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 test/parser/test-grub2-blscfg-default-index.c

On Thu, 2018-04-05 at 14:16 +0200, Javier Martinez Canillas wrote:
> When the BLS support was added, the conclusion was that default indexes
> didn't apply for BLS snippets. But for GRUB 2 the indexes refers to the
> boot menu entries in memory, regardless of how these were generated.
> 
> Since in GRUB 2 is valid to set a default index even for menu entries
> generated from BLS fragments, allow this to also be done in Petitboot.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> ---
> 
>  discover/grub2/blscfg.c                       | 17 +++++++++---
>  test/parser/Makefile.am                       |  1 +
>  test/parser/test-grub2-blscfg-default-index.c | 38 +++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+), 4 deletions(-)
>  create mode 100644 test/parser/test-grub2-blscfg-default-index.c
> 
> diff --git a/discover/grub2/blscfg.c b/discover/grub2/blscfg.c
> index 02ac621be061..654b33b7756e 100644
> --- a/discover/grub2/blscfg.c
> +++ b/discover/grub2/blscfg.c
> @@ -20,6 +20,7 @@
>  struct bls_state {
>  	struct discover_boot_option *opt;
>  	struct grub2_script *script;
> +	unsigned int idx;
>  	const char *filename;
>  	const char *title;
>  	const char *version;
> @@ -81,19 +82,25 @@ static void bls_process_pair(struct conf_context *conf, const char *name,
>  	}
>  }
>  
> -static bool option_is_default(struct grub2_script *script,
> +static bool option_is_default(struct bls_state *state,
>  			      struct boot_option *option)
>  {
> +	unsigned int idx;
>  	const char *var;
> +	char *end;
>  
> -	var = script_env_get(script, "default");
> +	var = script_env_get(state->script, "default");
>  	if (!var)
>  		return false;
>  
>  	if (!strcmp(var, option->id))
>  		return true;
>  
> -	return !strcmp(var, option->name);
> +	if (!strcmp(var, option->name))
> +		return true;
> +
> +	idx = strtoul(var, &end, 10);
> +	return !!(end != var && *end == '\0' && idx == state->idx);

I think we don't need the !! here?

>  }
>  
>  static void bls_finish(struct conf_context *conf)
> @@ -141,7 +148,7 @@ static void bls_finish(struct conf_context *conf)
>  		opt->dtb = create_grub2_resource(opt, conf->dc->device,
>  						 root, state->dtb);
>  
> -	option->is_default = option_is_default(state->script, option);
> +	option->is_default = option_is_default(state, option);
>  
>  	discover_context_add_boot_option(dc, opt);
>  
> @@ -179,6 +186,7 @@ int builtin_blscfg(struct grub2_script *script,
>  	struct dirent **bls_entries;
>  	struct conf_context *conf;
>  	struct bls_state *state;
> +	unsigned int current_idx = 0;

Without trying to over-complicate things, will a blscfg call in grub.cfg
only occur before normal GRUB boot options or can it be in between or
after? If the latter then would we need to set current_idx to the number
of boot options already parsed from this grub.cfg?

>  	char *buf, *filename;
>  	const char *blsdir;
>  	int n, len, rc = -1;
> @@ -216,6 +224,7 @@ int builtin_blscfg(struct grub2_script *script,
>  
>  		state->script = script;
>  		state->filename = filename;
> +		state->idx = current_idx++;
>  		conf->parser_info = state;
>  
>  		rc = parser_request_file(dc, dc->device, filename, &buf, &len);
> diff --git a/test/parser/Makefile.am b/test/parser/Makefile.am
> index 3479d88cdb23..6ff3972d8316 100644
> --- a/test/parser/Makefile.am
> +++ b/test/parser/Makefile.am
> @@ -41,6 +41,7 @@ parser_TESTS = \
>  	test/parser/test-grub2-test-file-ops \
>  	test/parser/test-grub2-single-yocto \
>  	test/parser/test-grub2-blscfg-default-filename \
> +	test/parser/test-grub2-blscfg-default-index \
>  	test/parser/test-grub2-blscfg-default-title \
>  	test/parser/test-grub2-blscfg-multiple-bls \
>  	test/parser/test-grub2-blscfg-opts-config \
> diff --git a/test/parser/test-grub2-blscfg-default-index.c b/test/parser/test-grub2-blscfg-default-index.c
> new file mode 100644
> index 000000000000..c2305700b4e4
> --- /dev/null
> +++ b/test/parser/test-grub2-blscfg-default-index.c
> @@ -0,0 +1,38 @@
> +#include "parser-test.h"
> +
> +#if 0 /* PARSER_EMBEDDED_CONFIG */
> +set default=1
> +blscfg
> +#endif
> +
> +void run_test(struct parser_test *test)
> +{
> +	struct discover_boot_option *opt;
> +	struct discover_context *ctx;
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.15.2-302.fc28.x86_64.conf",
> +			     "title Fedora (4.15.2-302.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "linux /vmlinuz-4.15.2-302.fc28.x86_64\n"
> +			     "initrd /initramfs-4.15.2-302.fc28.x86_64.img\n"
> +			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n\n");
> +
> +	test_add_file_string(test, test->ctx->device,
> +			     "/loader/entries/6c063c8e48904f2684abde8eea303f41-4.14.18-300.fc28.x86_64.conf",
> +			     "title Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)\n"
> +			     "linux /vmlinuz-4.14.18-300.fc28.x86_64\n"
> +			     "initrd /initramfs-4.14.18-300.fc28.x86_64.img\n"
> +			     "options root=/dev/mapper/fedora-root ro rd.lvm.lv=fedora/root\n");
> +
> +	test_read_conf_embedded(test, "/boot/grub2/grub.cfg");
> +
> +	test_run_parser(test, "grub2");
> +
> +	ctx = test->ctx;
> +
> +	opt = get_boot_option(ctx, 1);
> +
> +	check_name(opt, "Fedora (4.14.18-300.fc28.x86_64) 28 (Twenty Eight)");
> +
> +	check_is_default(opt);
> +}

On Tue, Apr 17, 2018 at 02:40:08PM +1000, Samuel Mendoza-Jonas wrote:
> > @@ -179,6 +186,7 @@ int builtin_blscfg(struct grub2_script *script,
> >  	struct dirent **bls_entries;
> >  	struct conf_context *conf;
> >  	struct bls_state *state;
> > +	unsigned int current_idx = 0;
> 
> Without trying to over-complicate things, will a blscfg call in grub.cfg
> only occur before normal GRUB boot options or can it be in between or
> after? If the latter then would we need to set current_idx to the number
> of boot options already parsed from this grub.cfg?

It can be in the middle of them, so I think you're correct.

On 04/17/2018 06:40 AM, Samuel Mendoza-Jonas wrote:
> On Thu, 2018-04-05 at 14:16 +0200, Javier Martinez Canillas wrote:

[snip]

>>  
>> -static bool option_is_default(struct grub2_script *script,
>> +static bool option_is_default(struct bls_state *state,
>>  			      struct boot_option *option)
>>  {
>> +	unsigned int idx;
>>  	const char *var;
>> +	char *end;
>>  
>> -	var = script_env_get(script, "default");
>> +	var = script_env_get(state->script, "default");
>>  	if (!var)
>>  		return false;
>>  
>>  	if (!strcmp(var, option->id))
>>  		return true;
>>  
>> -	return !strcmp(var, option->name);
>> +	if (!strcmp(var, option->name))
>> +		return true;
>> +
>> +	idx = strtoul(var, &end, 10);
>> +	return !!(end != var && *end == '\0' && idx == state->idx);
> 
> I think we don't need the !! here?
> 

Yeah, I know. I usually use it to make clear that's a boolean, but
I agree that makes the code excessively verbose for no good reason.

>>  }
>>  
>>  static void bls_finish(struct conf_context *conf)
>> @@ -141,7 +148,7 @@ static void bls_finish(struct conf_context *conf)
>>  		opt->dtb = create_grub2_resource(opt, conf->dc->device,
>>  						 root, state->dtb);
>>  
>> -	option->is_default = option_is_default(state->script, option);
>> +	option->is_default = option_is_default(state, option);
>>  
>>  	discover_context_add_boot_option(dc, opt);
>>  
>> @@ -179,6 +186,7 @@ int builtin_blscfg(struct grub2_script *script,
>>  	struct dirent **bls_entries;
>>  	struct conf_context *conf;
>>  	struct bls_state *state;
>> +	unsigned int current_idx = 0;
> 
> Without trying to over-complicate things, will a blscfg call in grub.cfg
> only occur before normal GRUB boot options or can it be in between or
> after? If the latter then would we need to set current_idx to the number
> of boot options already parsed from this grub.cfg?
> 

I'll fix this and also change the grub2-blscfg-default-index test case to
have both menu entries in the grub config and BLS fragments.

Something I noticed now when mixing them is that the Petitboot UI (at least
the ncurses based one) sorts the boot entries in the reverse order than I
would had expected. That is, the entry at the top is the older one and the
entry at the bottom is the latest one.

But both in the grub.cfg file and the grub UI, the boot menu entries are in
a descending order from newer to older. I wrongly assumed that was the case
in Petitboot as well. I'll include a fix so the menu entries are sorted as
expected by the Petitboot UI.

Best regards,