From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Juergen Borleis <jbe@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 2/2] uncompress: add Android sparse image support
Date: Sat, 16 Sep 2023 18:01:15 +0200 [thread overview]
Message-ID: <20230916160115.mp23mo2mxdfxfwix@pengutronix.de> (raw)
In-Reply-To: <20230914095859.22166-2-jbe@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 --]
next prev parent reply other threads:[~2023-09-16 16:03 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-14 9:58 [PATCH 1/2] sparse_image_fd: add function to re-use a file descriptor Juergen Borleis
2023-09-14 9:58 ` [PATCH 2/2] uncompress: add Android sparse image support Juergen Borleis
2023-09-16 16:01 ` Uwe Kleine-König [this message]
2023-09-21 12:04 ` Sascha Hauer
2023-09-21 12:01 ` Sascha Hauer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230916160115.mp23mo2mxdfxfwix@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=barebox@lists.infradead.org \
--cc=jbe@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox