Re: [PATCH 2/2] uncompress: add Android sparse image support

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

On Thu, Sep 14, 2023 at 11:58:59AM +0200, Juergen Borleis wrote:
> An Android sparse image file generated by the 'genimage' tool can be seen as a
> kind of compression, since it contains only the data which should really be
> written to a device.
> 
> This implementation writes only the intended content and skips anything else.
> 
> Signed-off-by: Juergen Borleis <jbe@pengutronix.de>

Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

while this obviously works, I still have a comment below:

> ---
>  lib/uncompress.c | 82 +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 78 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/uncompress.c b/lib/uncompress.c
> index 0608e9f..f17e9ce 100644
> --- a/lib/uncompress.c
> +++ b/lib/uncompress.c
> @@ -25,6 +25,10 @@
>  #include <malloc.h>
>  #include <fs.h>
>  #include <libfile.h>
> +#ifdef CONFIG_IMAGE_SPARSE
> +#include <image-sparse.h>
> +#include <linux/sizes.h>
> +#endif
>  
>  static void *uncompress_buf;
>  static unsigned int uncompress_size;
> @@ -60,6 +64,71 @@ static int uncompress_fill(void *buf, unsigned int len)
>  	return total;
>  }
>  
> +#ifdef CONFIG_IMAGE_SPARSE
> +static int uncompress_sparse(int source, int destination)
> +{
> +	int ret = 0;
> +	struct sparse_image_ctx *sparse;
> +
> +	/* rewind after checking the file type */
> +	lseek(source, 0, SEEK_SET);
> +
> +	sparse = sparse_image_fd(source);
> +	if (IS_ERR(sparse)) {
> +		pr_err("Failed to open or interpret sparse image file: %m\n");
> +		ret = 1;
> +		goto on_error_dev;
> +	}
> +
> +	const size_t transfer_buffer_length = SZ_128K;
> +	unsigned char *buf = malloc(transfer_buffer_length);
> +	if (!buf) {
> +		pr_err("Failed to alloc memory for the transfers\n");
> +		ret = 1;
> +		goto on_error_sparse;
> +	}
> +
> +	while (1) {
> +		loff_t write_offset;
> +		size_t write_length;
> +
> +		int rc = sparse_image_read(sparse, buf, &write_offset, transfer_buffer_length, &write_length);
> +		if (rc) {
> +			ret = 1;
> +			goto on_error_memory;
> +		}
> +		if (!write_length)
> +			break;
> +
> +		discard_range(destination, write_length, write_offset);
> +
> +		write_offset = lseek(destination, write_offset, SEEK_SET);
> +		if (write_offset == -1) {
> +			pr_err("Failed to set next data's destination offset: %m\n");
> +			ret = 1;
> +			goto on_error_memory;
> +
> +		}
> +
> +		rc = write_full(destination, buf, write_length);
> +		if (rc < 0) {
> +			pr_err("Failed to write destination's next data: %m\n");
> +			ret = 1;
> +			goto on_error_memory;
> +		}
> +	}
> +
> +on_error_memory:
> +	free(buf);
> +on_error_sparse:
> +	free(sparse); /* Note: sparse_image_close(sparse); would also close the input file descriptor */
> +on_error_dev:
> +	return ret;
> +}
> +#endif
> +
> +static int uncompress_infd, uncompress_outfd;
> +
>  int uncompress(unsigned char *inbuf, int len,
>  	   int(*fill)(void*, unsigned int),
>  	   int(*flush)(void*, unsigned int),
> @@ -121,6 +190,10 @@ int uncompress(unsigned char *inbuf, int len,
>  	case filetype_xz_compressed:
>  		compfn = decompress_unxz;
>  		break;
> +#endif
> +#ifdef CONFIG_IMAGE_SPARSE
> +	case filetype_android_sparse:
> +		break;
>  #endif
>  	default:
>  		err = basprintf("cannot handle filetype %s",
> @@ -131,16 +204,17 @@ int uncompress(unsigned char *inbuf, int len,
>  		goto err;
>  	}
>  
> -	ret = compfn(inbuf, len, fill ? uncompress_fill : NULL,
> -			flush, output, pos, error_fn);
> +	if (ft == filetype_android_sparse)
> +		ret = uncompress_sparse(uncompress_infd, uncompress_outfd);
> +	else
> +		ret = compfn(inbuf, len, fill ? uncompress_fill : NULL,
> +			     flush, output, pos, error_fn);

Wouldn't it be more natural to make uncompress_sparse use the same
prototype as the other uncompress functions and then just have:

#ifdef CONFIG_IMAGE_SPARSE
	case filetype_android_sparse:
		compfn = decompress_sparse;
		break;
#endif

Without the need to adapt the compfn call?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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