[OpenBIOS] [PATCH] bootinfo_init_program: improve memory allocation efficiency

Mark Cave-Ayland mark.cave-ayland at ilande.co.uk
Mon Apr 20 21:36:13 CEST 2015


On 19/04/15 22:36, Cormac O'Brien wrote:

> Here's v2, with the debug macro untouched.
> 
> ---
>  libopenbios/bootinfo_load.c | 100 +++++++++++++++++++++++++++++---------------
>  1 file changed, 66 insertions(+), 34 deletions(-)
> 
> diff --git a/libopenbios/bootinfo_load.c b/libopenbios/bootinfo_load.c
> index fa9e36b..855b5dc 100644
> --- a/libopenbios/bootinfo_load.c
> +++ b/libopenbios/bootinfo_load.c
> @@ -134,12 +134,13 @@ bootinfo_init_program(void)
>  	char *base;
>  	int proplen;
>  	phandle_t chosen;
> -	int tag, taglen, script, scriptlen, scriptvalid, entity, chrp;
> +	int tag, taglen, script, scriptind, scriptlen, scriptstart, scriptvalid,
> +		entity, chrp;

Whitespace here.

>  	char tagbuf[128], c;
>  	char *device, *filename, *directory, *partition;
>  	int current, size;
>  	char *bootscript;
> -        char *tmp;
> +	char *tmp;

... and here.

>  	char bootpath[1024];
>  
>  	/* Parse the boot script */
> @@ -158,21 +159,14 @@ bootinfo_init_program(void)
>  	feval("load-base");
>  	base = (char*)cell2pointer(POP());
>  
> -	feval("load-size");
> -	size = POP();
> -
> -	bootscript = malloc(size);
> -	if (bootscript == NULL) {
> -		DPRINTF("Can't malloc %d bytes\n", size);
> -		return;
> -	}
> -
>  	if (!is_bootinfo(base)) {
>  		DPRINTF("Not a valid bootinfo memory image\n");
> -                free(bootscript);
>  		return;
>  	}
>  
> +	feval("load-size");
> +	size = POP();
> +
>  	chrp = 0;
>  	tag = 0;
>  	taglen = 0;
> @@ -198,16 +192,10 @@ bootinfo_init_program(void)
>  				if (strncasecmp(tagbuf, "boot-script", 11) == 0) {
>  					script = 1;
>  					scriptlen = 0;
> +					scriptstart = current;
>  				} else if (strncasecmp(tagbuf, "/boot-script", 12) == 0) {
> -
>  					script = 0;
> -					bootscript[scriptlen] = '\0';
> -
> -					DPRINTF("got bootscript %s\n",
> -						bootscript);
> -
>  					scriptvalid = -1;
> -
>  					break;
>  				} else if (strncasecmp(tagbuf, "/chrp-boot", 10) == 0)
>  					break;
> @@ -217,42 +205,86 @@ bootinfo_init_program(void)
>  		} else if (script && c == '&') {
>  			entity = 1;
>  			taglen = 0;
> -		} else if (entity && c ==';') {
> +		} else if (entity && c == ';') {
>  			entity = 0;
>  			tagbuf[taglen] = '\0';
>  			if (strncasecmp(tagbuf, "lt", 2) == 0) {
> -				bootscript[scriptlen++] = '<';
> +				scriptlen++;
>  			} else if (strncasecmp(tagbuf, "gt", 2) == 0) {
> -				bootscript[scriptlen++] = '>';
> +				scriptlen++;
>  			} else if (strncasecmp(tagbuf, "device", 6) == 0) {
> -				strcpy(bootscript + scriptlen, device);
>  				scriptlen += strlen(device);
>  			} else if (strncasecmp(tagbuf, "partition", 9) == 0) {
> -				strcpy(bootscript + scriptlen, partition);
>  				scriptlen += strlen(partition);
>  			} else if (strncasecmp(tagbuf, "directory", 9) == 0) {
> -				strcpy(bootscript + scriptlen, directory);
>  				scriptlen += strlen(directory);
>  			} else if (strncasecmp(tagbuf, "filename", 8) == 0) {
> -				strcpy(bootscript + scriptlen, filename);
>  				scriptlen += strlen(filename);
>  			} else if (strncasecmp(tagbuf, "full-path", 9) == 0) {
> -				strcpy(bootscript + scriptlen, bootpath);
>  				scriptlen += strlen(bootpath);
> -			} else { /* unknown, keep it */
> -				bootscript[scriptlen] = '&';
> -				strcpy(bootscript + scriptlen + 1, tagbuf);
> -				scriptlen += taglen + 1;
> -				bootscript[scriptlen] = ';';
> -				scriptlen++;
> +			} else {
> +				scriptlen += taglen + 2;
>  			}
>  		} else if (entity && taglen < sizeof(tagbuf)) {
>  			tagbuf[taglen++] = c;
>  		} else if (script && scriptlen < size) {
> -			bootscript[scriptlen++] = c;
> +			scriptlen++;
> +		}
> +	}
> +
> +    bootscript = malloc(scriptlen * sizeof *bootscript + 1);
> +	if (bootscript == NULL) {
> +		DPRINTF("Can't malloc %d bytes\n", size);
> +		return;
> +	}
> +
> +	entity = 0;
> +	scriptind = 0;
> +	taglen = 0;
> +	current = scriptstart;
> +	while (current < scriptstart + scriptlen) {
> +		c = base[current++];
> +
> +		if (c == '&') {
> +			entity = 1;
> +			taglen = 0;
> +		} else if (entity && c == ';') {
> +			entity = 0;
> +			tagbuf[taglen] = '\0';
> +			if (strncasecmp(tagbuf, "lt", 2) == 0) {
> +				bootscript[scriptind++] = '<';
> +			} else if (strncasecmp(tagbuf, "gt", 2) == 0) {
> +				bootscript[scriptind++] = '>';
> +			} else if (strncasecmp(tagbuf, "device", 6) == 0) {
> +				strcpy(bootscript + scriptind, device);
> +				scriptind += strlen(device);
> +			} else if (strncasecmp(tagbuf, "partition", 9) == 0) {
> +				strcpy(bootscript + scriptind, partition);
> +				scriptind += strlen(partition);
> +			} else if (strncasecmp(tagbuf, "directory", 9) == 0) {
> +				strcpy(bootscript + scriptind, directory);
> +				scriptind += strlen(directory);
> +			} else if (strncasecmp(tagbuf, "filename", 8) == 0) {
> +				strcpy(bootscript + scriptind, filename);
> +				scriptlen += strlen(filename);
> +			} else if (strncasecmp(tagbuf, "full-path", 9) == 0) {
> +				strcpy(bootscript + scriptind, bootpath);
> +				scriptlen += strlen(bootscript);
> +			} else { /* unknown, keep it */
> +				bootscript[scriptind++] = '&';
> +				strcpy(bootscript + scriptind, tagbuf);
> +				scriptind += taglen;
> +				bootscript[scriptind++] = ';';
> +			}
> +		} else if (entity && taglen < sizeof(tagbuf)) {
> +			tagbuf[taglen++] = c;
> +		} else {
> +			bootscript[scriptind++] = c;
>  		}
>  	}
>  
> +	bootscript[scriptlen] = '\0';
> +
>  	/* If the payload is bootinfo then we execute it immediately */
>  	if (scriptvalid) {
>  		DPRINTF("bootscript: %s\n", bootscript);
> 

I haven't actually reworked the patch by hand in order to be able to
apply directly, but the overall concept looks like it should work.
Similar comments about whitespace and subject lines from the previous
patch are also relevant here too - maybe libopenbios or bootcode_load.c
as a subject prefix if you want to be a bit more more specific?

Part of me feels that this approach still requires quite a bit of
refactoring of the existing code to accommodate this change. Thinking
more succinctly, could the same effect be accomplished with something
like this before the main while() loop (given that we know the entities
will always take *less* space and EOT will never appear unencoded in the
final output):

    feval("load-size");
    size = POP();

    ...

    for (scriptlen = 0; scriptlen < size && base[scriptlen] != 0x4;
scriptlen++);

    ...

Also a comment explaining that some boot scripts such as OS 9 add a
binary payload after the chrp-boot script would be useful to explain to
the casual code-dweller exactly why we are scanning the entire boot
script as retrieved from disk first.


ATB,

Mark.




More information about the OpenBIOS mailing list