Re: EXTERNAL - [PATCH v1] Add bootchooser command option for active boot target

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Markus,

(Adding the list back to Cc)

On 14.04.23 12:49, Burri Markus LabTec wrote:
> Hello Ahmad
> 
> Here is example script:
> 
> #!/bin/sh
> bootchooser -n
> #Execute all hook scripts found on active target
> HOOKDIR="/mnt/${nv.bootchooser.active_boot}/boot/barebox-hooks"
> cd ${HOOKDIR}
> for HOOK in *; do
>     echo "run barebox-hook: \'${HOOK}\'"
>     . ${HOOKDIR}/${HOOK}
> done
> cd /

I assume this is an init script? This script will execute
the hooks of the old rootfs once before booting the new rootfs.

This doesn't sound like a good idea.

Fundamentally, the barebox environment is not power-fail safe,
that's why bootchooser is using barebox-state. So writing nv
variables without user interaction is not ok.

Did you know that you can have scripts as bootchooser targets?
Instead of e.g. bootchooser.system0.boot=mmc0.1, you can add
/env/boot/system0 and then do whatever you want there before 
calling boot mmc0. Would that suffice?

Cheers,
Ahmad

 
> -----Original Message-----
> From: Ahmad Fatoum <a.fatoum@pengutronix.de> 
> Sent: Freitag, 14. April 2023 11:49
> To: Burri Markus LabTec <Markus.Burri@mt.com>; barebox@lists.infradead.org
> Subject: Re: EXTERNAL - [PATCH v1] Add bootchooser command option for active boot target
> 
> Hello Markus,
> 
> On 14.04.23 11:40, Markus Burri wrote:
>> Add bootchooser command option to extract and save the active target.
>> The information about active target are stored into two variables 
>> (bootchooser.active_target and bootchooser.active_boot) This 
>> information may be used into scripts to do actions related to the 
>> active target before starting it.
> 
> Can you share an example script that depends on your new bootchooser -n feature to aid review?
> 
>>
>> Signed-off-by: Markus Burri <markus.burri@mt.com>
>>
>> Gbp-Pq: Name 0058-Add-command-for-next-boot-target.patch
>> ---
>>  Documentation/user/bootchooser.rst |  6 ++++++
>>  commands/bootchooser.c             | 12 +++++++++++-
>>  common/bootchooser.c               | 21 +++++++++++++++++++++
>>  include/bootchooser.h              |  1 +
>>  4 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/user/bootchooser.rst 
>> b/Documentation/user/bootchooser.rst
>> index 8456e11..68ec1b6 100644
>> --- a/Documentation/user/bootchooser.rst
>> +++ b/Documentation/user/bootchooser.rst
>> @@ -168,6 +168,12 @@ options not specific to any boot target.
>>    set of ``global.bootchooser.<targetname>.<variablename>`` variables must exist.
>>  ``global.bootchooser.last_chosen``
>>    *bootchooser* sets this to the boot target that was chosen on last boot (index).
>> +``global.bootchooser.active_target``
>> +  Set to the active boot target that will boot next.
>> +  Call *bootchooser -n* to update the value.
>> +``global.bootchooser.active_boot``
>> +  Set to the boot device of the active target that will boot next. 
>> +  Call *bootchooser -n* to update the value.
>>  
>>  .. _bootchooser,setup_example:
>>  
>> diff --git a/commands/bootchooser.c b/commands/bootchooser.c index 
>> ac763a6..20bd2d6 100644
>> --- a/commands/bootchooser.c
>> +++ b/commands/bootchooser.c
>> @@ -59,8 +59,9 @@ static int do_bootchooser(int argc, char *argv[])
>>  	int info = 0;
>>  	bool done_something = false;
>>  	bool last_boot_successful = false;
>> +	bool active_boot_target = false;
>>  
>> -	while ((opt = getopt(argc, argv, "a:p:is")) > 0) {
>> +	while ((opt = getopt(argc, argv, "a:p:ins")) > 0) {
>>  		switch (opt) {
>>  		case 'a':
>>  			if (!strcmp(optarg, "default"))
>> @@ -77,6 +78,9 @@ static int do_bootchooser(int argc, char *argv[])
>>  		case 'i':
>>  			info = 1;
>>  			break;
>> +		case 'n':
>> +			active_boot_target = true;
>> +			break;
>>  		case 's':
>>  			last_boot_successful = true;
>>  			break;
>> @@ -120,6 +124,11 @@ static int do_bootchooser(int argc, char *argv[])
>>  		done_something = true;
>>  	}
>>  
>> +	if (active_boot_target) {
>> +		bootchooser_active_target(bootchooser);
>> +		done_something = true;
>> +	}
>> +
>>  	if (!done_something) {
>>  		printf("Nothing to do\n");
>>  		ret = COMMAND_ERROR_USAGE;
>> @@ -137,6 +146,7 @@ BAREBOX_CMD_HELP_TEXT("Options:")  
>> BAREBOX_CMD_HELP_OPT ("-a <n|default> [TARGETS]",  "set remaining 
>> attempts of given targets to 'n' or the default attempts")  
>> BAREBOX_CMD_HELP_OPT ("-p <n|default> [TARGETS]",  "set priority of 
>> given targets to 'n' or the default priority")  BAREBOX_CMD_HELP_OPT 
>> ("-i",  "Show information about the bootchooser")
>> +BAREBOX_CMD_HELP_OPT ("-n",  "Save active boot target to 
>> +bootchooser.active_target and active boot device to 
>> +bootchooser.active_boot")
>>  BAREBOX_CMD_HELP_OPT ("-s",  "Mark the last boot successful")  
>> BAREBOX_CMD_HELP_END
>>  
>> diff --git a/common/bootchooser.c b/common/bootchooser.c index 
>> c08db03..1ec29e6 100644
>> --- a/common/bootchooser.c
>> +++ b/common/bootchooser.c
>> @@ -619,6 +619,27 @@ void bootchooser_info(struct bootchooser *bc)
>>  	       bc->last_chosen->name : "unknown");  }
>>  
>> +/**
>> + * bootchooser_active_target - Save information about active boot 
>> +target
>> + * @bc: The bootchooser
>> + */
>> +void bootchooser_active_target(struct bootchooser *bc) {
>> +	struct bootchooser_target *target;
>> +
>> +	list_for_each_entry(target, &bc->targets, list) {
>> +		if (bootchooser_target_ok(target, NULL)) {
>> +			if (bc->verbose) {
>> +				pr_info("bootchooser.active_target=%s\n", target->name);
>> +				pr_info("bootchooser.active_boot=%s\n", target->boot);
>> +			}
>> +			nvvar_add("bootchooser.active_target", target->name);
>> +			nvvar_add("bootchooser.active_boot", target->boot);
>> +			return;
>> +		}
>> +	}
>> +}
>> +
>>  /**
>>   * bootchooser_get_target - get the target that shall be booted next
>>   * @bc:		The bootchooser
>> diff --git a/include/bootchooser.h b/include/bootchooser.h index 
>> 7822c01..9825593 100644
>> --- a/include/bootchooser.h
>> +++ b/include/bootchooser.h
>> @@ -12,6 +12,7 @@ int bootchooser_save(struct bootchooser 
>> *bootchooser);  int bootchooser_put(struct bootchooser *bootchooser);
>>  
>>  void bootchooser_info(struct bootchooser *bootchooser);
>> +void bootchooser_active_target(struct bootchooser *bc);
>>  
>>  int bootchooser_boot(struct bootchooser *bc);
>>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |