mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators
@ 2020-02-12 14:16 Ahmad Fatoum
  2020-02-12 14:16 ` [PATCH 2/2] commands: boot: create boot entries on demand Ahmad Fatoum
  2020-02-14  7:56 ` [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Sascha Hauer
  0 siblings, 2 replies; 4+ messages in thread
From: Ahmad Fatoum @ 2020-02-12 14:16 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

We currently create all boot entries before attempting boot. This is
less than optimal, because this may involve probing devices that won't
be used for actual boot.

In preparation for changing this, refactor the code, so we only have one
loop we need to touch.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/boot.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/commands/boot.c b/commands/boot.c
index 0257b3dd4f18..ce4eeac6534c 100644
--- a/commands/boot.c
+++ b/commands/boot.c
@@ -21,13 +21,29 @@
 
 #include <linux/stat.h>
 
+static char *next_argv(void *context)
+{
+	char ***argv = context;
+	char *next = **argv;
+	(*argv)++;
+	return next;
+}
+
+static char *next_word(void *context)
+{
+	return strsep(context, " ");
+}
+
 static int do_boot(int argc, char *argv[])
 {
 	char *freep = NULL;
 	int opt, ret = 0, do_list = 0, do_menu = 0;
-	int i, dryrun = 0, verbose = 0, timeout = -1;
+	int dryrun = 0, verbose = 0, timeout = -1;
 	struct bootentries *entries;
 	struct bootentry *entry;
+	void *handle;
+	const char *name;
+	char *(*next)(void *);
 
 	while ((opt = getopt(argc, argv, "vldmt:w:")) > 0) {
 		switch (opt) {
@@ -54,31 +70,26 @@ static int do_boot(int argc, char *argv[])
 		}
 	}
 
-	entries = bootentries_alloc();
-
 	if (optind < argc) {
-		for (i = optind; i < argc; i++) {
-			ret = bootentry_create_from_name(entries, argv[i]);
-			if (ret <= 0)
-				printf("Nothing bootable found on '%s'\n", argv[i]);
-	       }
+		handle = &argv[optind];
+		next = next_argv;
 	} else {
 		const char *def;
-		char *sep, *name;
 
 		def = getenv("global.boot.default");
 		if (!def)
 			return 0;
 
-		sep = freep = xstrdup(def);
+		handle = freep = xstrdup(def);
+		next = next_word;
+	}
 
-		while ((name = strsep(&sep, " ")) != NULL) {
-			ret = bootentry_create_from_name(entries, name);
-			if (ret <= 0)
-				printf("Nothing bootable found on '%s'\n", name);
-		}
+	entries = bootentries_alloc();
 
-		free(freep);
+	while ((name = next(&handle)) != NULL) {
+		ret = bootentry_create_from_name(entries, name);
+		if (ret <= 0)
+			printf("Nothing bootable found on '%s'\n", name);
 	}
 
 	if (list_empty(&entries->entries)) {
@@ -104,6 +115,7 @@ static int do_boot(int argc, char *argv[])
 
 out:
 	bootentries_free(entries);
+	free(freep);
 
 	return ret;
 }
-- 
2.25.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH 2/2] commands: boot: create boot entries on demand
  2020-02-12 14:16 [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Ahmad Fatoum
@ 2020-02-12 14:16 ` Ahmad Fatoum
  2020-02-14  7:56 ` [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Sascha Hauer
  1 sibling, 0 replies; 4+ messages in thread
From: Ahmad Fatoum @ 2020-02-12 14:16 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

We currently create all boot entries before attempting boot. This is
less than optimal, because this may involve probing devices that won't
be used for actual boot.

Fix this by not creating boot entries till the previous boot argument
(command line argument or boot.default word) was found to be unbootable.

This means that "boot mmc1 mmc0" will now not touch mmc0 if mmc1 had a
bootable entry. This is only done when no menu or list was requested.

As the boot entries are in a linked list, the allocation done for each
boot argument could be omitted, but as the saving from skipping an
allocation is easily dwarfed by the boot medium access, we just
reallocate and enjoy the improved code clarity.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/boot.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/commands/boot.c b/commands/boot.c
index ce4eeac6534c..aeaba3992e1d 100644
--- a/commands/boot.c
+++ b/commands/boot.c
@@ -90,6 +90,18 @@ static int do_boot(int argc, char *argv[])
 		ret = bootentry_create_from_name(entries, name);
 		if (ret <= 0)
 			printf("Nothing bootable found on '%s'\n", name);
+
+		if (do_list || do_menu)
+			continue;
+
+		bootentries_for_each_entry(entries, entry) {
+			ret = boot_entry(entry, verbose, dryrun);
+			if (!ret)
+				break;
+		}
+
+		bootentries_free(entries);
+		entries = bootentries_alloc();
 	}
 
 	if (list_empty(&entries->entries)) {
@@ -107,12 +119,6 @@ static int do_boot(int argc, char *argv[])
 		goto out;
 	}
 
-	bootentries_for_each_entry(entries, entry) {
-		ret = boot_entry(entry, verbose, dryrun);
-		if (!ret)
-			break;
-	}
-
 out:
 	bootentries_free(entries);
 	free(freep);
-- 
2.25.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators
  2020-02-12 14:16 [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Ahmad Fatoum
  2020-02-12 14:16 ` [PATCH 2/2] commands: boot: create boot entries on demand Ahmad Fatoum
@ 2020-02-14  7:56 ` Sascha Hauer
  2020-02-14  8:12   ` Ahmad Fatoum
  1 sibling, 1 reply; 4+ messages in thread
From: Sascha Hauer @ 2020-02-14  7:56 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Wed, Feb 12, 2020 at 03:16:37PM +0100, Ahmad Fatoum wrote:
> We currently create all boot entries before attempting boot. This is
> less than optimal, because this may involve probing devices that won't
> be used for actual boot.
> 
> In preparation for changing this, refactor the code, so we only have one
> loop we need to touch.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  commands/boot.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/commands/boot.c b/commands/boot.c
> index 0257b3dd4f18..ce4eeac6534c 100644
> --- a/commands/boot.c
> +++ b/commands/boot.c
> @@ -21,13 +21,29 @@
>  
>  #include <linux/stat.h>
>  
> +static char *next_argv(void *context)
> +{
> +	char ***argv = context;
> +	char *next = **argv;
> +	(*argv)++;
> +	return next;
> +}
> +
> +static char *next_word(void *context)
> +{
> +	return strsep(context, " ");
> +}
> +
>  static int do_boot(int argc, char *argv[])
>  {
>  	char *freep = NULL;
>  	int opt, ret = 0, do_list = 0, do_menu = 0;
> -	int i, dryrun = 0, verbose = 0, timeout = -1;
> +	int dryrun = 0, verbose = 0, timeout = -1;
>  	struct bootentries *entries;
>  	struct bootentry *entry;
> +	void *handle;
> +	const char *name;
> +	char *(*next)(void *);
>  
>  	while ((opt = getopt(argc, argv, "vldmt:w:")) > 0) {
>  		switch (opt) {
> @@ -54,31 +70,26 @@ static int do_boot(int argc, char *argv[])
>  		}
>  	}
>  
> -	entries = bootentries_alloc();
> -
>  	if (optind < argc) {
> -		for (i = optind; i < argc; i++) {
> -			ret = bootentry_create_from_name(entries, argv[i]);
> -			if (ret <= 0)
> -				printf("Nothing bootable found on '%s'\n", argv[i]);
> -	       }
> +		handle = &argv[optind];
> +		next = next_argv;
>  	} else {
>  		const char *def;
> -		char *sep, *name;
>  
>  		def = getenv("global.boot.default");
>  		if (!def)
>  			return 0;
>  
> -		sep = freep = xstrdup(def);
> +		handle = freep = xstrdup(def);
> +		next = next_word;
> +	}
>  
> -		while ((name = strsep(&sep, " ")) != NULL) {
> -			ret = bootentry_create_from_name(entries, name);
> -			if (ret <= 0)
> -				printf("Nothing bootable found on '%s'\n", name);
> -		}
> +	entries = bootentries_alloc();
>  
> -		free(freep);
> +	while ((name = next(&handle)) != NULL) {
> +		ret = bootentry_create_from_name(entries, name);
> +		if (ret <= 0)
> +			printf("Nothing bootable found on '%s'\n", name);
>  	}
>  
>  	if (list_empty(&entries->entries)) {
> @@ -104,6 +115,7 @@ static int do_boot(int argc, char *argv[])
>  
>  out:
>  	bootentries_free(entries);
> +	free(freep);
>  
>  	return ret;
>  }
> -- 
> 2.25.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators
  2020-02-14  7:56 ` [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Sascha Hauer
@ 2020-02-14  8:12   ` Ahmad Fatoum
  0 siblings, 0 replies; 4+ messages in thread
From: Ahmad Fatoum @ 2020-02-14  8:12 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox



On 2/14/20 8:56 AM, Sascha Hauer wrote:
> On Wed, Feb 12, 2020 at 03:16:37PM +0100, Ahmad Fatoum wrote:
>> We currently create all boot entries before attempting boot. This is
>> less than optimal, because this may involve probing devices that won't
>> be used for actual boot.
>>
>> In preparation for changing this, refactor the code, so we only have one
>> loop we need to touch.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  commands/boot.c | 44 ++++++++++++++++++++++++++++----------------
>>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> Applied, thanks

Relooking at the diff, I spotted a small issue that would happen with
dryrun boot. I sent a fixup.

> 
> Sascha
> 
>>
>> diff --git a/commands/boot.c b/commands/boot.c
>> index 0257b3dd4f18..ce4eeac6534c 100644
>> --- a/commands/boot.c
>> +++ b/commands/boot.c
>> @@ -21,13 +21,29 @@
>>  
>>  #include <linux/stat.h>
>>  
>> +static char *next_argv(void *context)
>> +{
>> +	char ***argv = context;
>> +	char *next = **argv;
>> +	(*argv)++;
>> +	return next;
>> +}
>> +
>> +static char *next_word(void *context)
>> +{
>> +	return strsep(context, " ");
>> +}
>> +
>>  static int do_boot(int argc, char *argv[])
>>  {
>>  	char *freep = NULL;
>>  	int opt, ret = 0, do_list = 0, do_menu = 0;
>> -	int i, dryrun = 0, verbose = 0, timeout = -1;
>> +	int dryrun = 0, verbose = 0, timeout = -1;
>>  	struct bootentries *entries;
>>  	struct bootentry *entry;
>> +	void *handle;
>> +	const char *name;
>> +	char *(*next)(void *);
>>  
>>  	while ((opt = getopt(argc, argv, "vldmt:w:")) > 0) {
>>  		switch (opt) {
>> @@ -54,31 +70,26 @@ static int do_boot(int argc, char *argv[])
>>  		}
>>  	}
>>  
>> -	entries = bootentries_alloc();
>> -
>>  	if (optind < argc) {
>> -		for (i = optind; i < argc; i++) {
>> -			ret = bootentry_create_from_name(entries, argv[i]);
>> -			if (ret <= 0)
>> -				printf("Nothing bootable found on '%s'\n", argv[i]);
>> -	       }
>> +		handle = &argv[optind];
>> +		next = next_argv;
>>  	} else {
>>  		const char *def;
>> -		char *sep, *name;
>>  
>>  		def = getenv("global.boot.default");
>>  		if (!def)
>>  			return 0;
>>  
>> -		sep = freep = xstrdup(def);
>> +		handle = freep = xstrdup(def);
>> +		next = next_word;
>> +	}
>>  
>> -		while ((name = strsep(&sep, " ")) != NULL) {
>> -			ret = bootentry_create_from_name(entries, name);
>> -			if (ret <= 0)
>> -				printf("Nothing bootable found on '%s'\n", name);
>> -		}
>> +	entries = bootentries_alloc();
>>  
>> -		free(freep);
>> +	while ((name = next(&handle)) != NULL) {
>> +		ret = bootentry_create_from_name(entries, name);
>> +		if (ret <= 0)
>> +			printf("Nothing bootable found on '%s'\n", name);
>>  	}
>>  
>>  	if (list_empty(&entries->entries)) {
>> @@ -104,6 +115,7 @@ static int do_boot(int argc, char *argv[])
>>  
>>  out:
>>  	bootentries_free(entries);
>> +	free(freep);
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.25.0
>>
>>
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>>
> 

-- 
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 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-14  8:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 14:16 [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Ahmad Fatoum
2020-02-12 14:16 ` [PATCH 2/2] commands: boot: create boot entries on demand Ahmad Fatoum
2020-02-14  7:56 ` [PATCH 1/2] commands: boot: refactor boot entry creation to use iterators Sascha Hauer
2020-02-14  8:12   ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox