Re: [PATCH v1 1/2] util: Only read files until fdt_totalsize

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]

On Tue, Sep 24, 2024 at 04:50:51PM +0200, Andreas Gnau wrote:
> When reading files, it is actually only necessary to read the amount of
> bytes specified in the FDT header. Any trailing data that is not part of
> the FDT is not relevant to the operation of any of the utilities and is
> discarded anyways. Stop reading when reaching either fdt_totalsize bytes
> or end-of-file.
> 
> Stopping reading before end-of-file saves a considerable amount of
> memory when using FIT (Flattened Image Tree) [1] images with "external
> data" which are used by bootloaders such as Das U-Boot. In case of FIT
> images, the FDT is a few kilobytes, while the actual file can be several
> megabytes, which is quite a significant difference in memory usage.
> 
> [1] https://fitspec.osfw.foundation/
> 
> Signed-off-by: Andreas Gnau <andreas.gnau@iopsys.eu>

The idea is good, but the approach needs some work.

First, utilfdt_read_err() wasn't intended to be specific to loading a
dtb, although that happens to be all we use it for.  So I'd prefer to
see this replace by a new function named to indicate it's specifically
for loading dtbs.

Secondly, folding the totalsize logic into the existing loop is
unnecessarily complex.  If we're basing the load size on the totalsize
field, there's a much more straightforward approach:

 1. Load the (v1) header into a temporary buffer
 2. Check the magic number (to see if it really is a dtb)
 3. Allocate a final buffer based on totalsize, copy the header into it
 4. Read the remaining (totalsize - header) bytes into the buffer

Both (1) and (4) can be done with a classic "read_all()" helper to
deal with short read()s.

> ---
>  util.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/util.c b/util.c
> index 507f0120cd13..093303078d5f 100644
> --- a/util.c
> +++ b/util.c
> @@ -249,6 +249,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
>  	char *buf = NULL;
>  	size_t bufsize = 1024, offset = 0;
>  	int ret = 0;
> +	uint32_t totalsize = UINT32_MAX;
>  
>  	*buffp = NULL;
>  	if (strcmp(filename, "-") != 0) {
> @@ -257,7 +258,7 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
>  			return errno;
>  	}
>  
> -	/* Loop until we have read everything */
> +	/* Loop until we have read until the end of file or end of FDT */
>  	buf = xmalloc(bufsize);
>  	do {
>  		/* Expand the buffer to hold the next chunk */
> @@ -272,14 +273,24 @@ int utilfdt_read_err(const char *filename, char **buffp, size_t *len)
>  			break;
>  		}
>  		offset += ret;
> -	} while (ret != 0);
> +
> +		/* assumes that the size is always stored in V1 part */
> +		if (totalsize == UINT32_MAX && offset >= FDT_V1_SIZE) {
> +			totalsize = fdt_totalsize(buf);
> +			if (totalsize <= 0)
> +				/* read file up to INT32_MAX in case of errors */
> +				totalsize = UINT32_MAX;
> +		}
> +	} while (ret != 0 && offset < totalsize);
>  
>  	/* Clean up, including closing stdin; return errno on error */
>  	close(fd);
> -	if (ret)
> +	if (ret <= 0)
>  		free(buf);
> -	else
> +	else {
>  		*buffp = buf;
> +		ret = 0;
> +	}
>  	if (len)
>  		*len = bufsize;
>  	return ret;

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]