From: Sascha Hauer <sha@pengutronix.de>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Juergen Borleis <jbe@pengutronix.de>, barebox@lists.infradead.org
Subject: Re: [PATCH 2/2] uncompress: add Android sparse image support
Date: Thu, 21 Sep 2023 14:04:19 +0200 [thread overview]
Message-ID: <20230921120419.GJ637806@pengutronix.de> (raw)
In-Reply-To: <20230916160115.mp23mo2mxdfxfwix@pengutronix.de>
On Sat, Sep 16, 2023 at 06:01:15PM +0200, Uwe Kleine-König wrote:
> 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?
compfn() doesn't have access to the file descriptors which the current
sparse implementation needs.
It would be great though to let our sparse image support use an input
buffer and a fill() function.
Sascha
--
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 |
next prev parent reply other threads:[~2023-09-21 12:05 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
2023-09-21 12:04 ` Sascha Hauer [this message]
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=20230921120419.GJ637806@pengutronix.de \
--to=sha@pengutronix.de \
--cc=barebox@lists.infradead.org \
--cc=jbe@pengutronix.de \
--cc=u.kleine-koenig@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