On Wed, Feb 25, 2009 at 02:43:14PM +0100, Geert Uytterhoeven wrote:
> Modify SquashFS 4 to use the new "pcomp" crypto interface for decompression,
> instead of calling the underlying zlib library directly. This simplifies e.g.
> the addition of support for hardware decompression and different decompression
> algorithms.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Cc: Phillip Lougher <[email protected]>
I've applied patches 1-5. I'd like to see an ack on this before
applying it.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu wrote:
> On Wed, Feb 25, 2009 at 02:43:14PM +0100, Geert Uytterhoeven wrote:
>> Modify SquashFS 4 to use the new "pcomp" crypto interface for decompression,
>> instead of calling the underlying zlib library directly. This simplifies e.g.
>> the addition of support for hardware decompression and different decompression
>> algorithms.
>>
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> Cc: Phillip Lougher <[email protected]>
>
> I've applied patches 1-5. I'd like to see an ack on this before
> applying it.
>
I've not acked it because I've not yet made any decisions as to whether
I want it in Squashfs. Also as Squashfs maintainer I maintain my own
tree of patches to Squashfs, and so I'll prefer to add it to my tree for
subsequent feeding into 2.6.30 once the merge window opens.
I'm fairly agnostic about what decompression library Squashfs uses. The
only thing I care about is cleanliness and usability of the API and
performance. If the crypto API is cleaner or the cryto zlib
implementation is faster than the current zlib implementation then I see
no reason not to move over to it. But, I've seen no performance figures
and the API seems clumsier.
Two API issues of concern (one major, one minor). Both of these relate
to the way Squashfs drives the decompression code, where it repeatedly
calls it supplying additional input/output buffers, rather than using a
"single-shot" approach where it calls the decompression code once
supplying all the necessary input and output buffer space.
1. Minor issue -the lack of a stream.total_out field. The current
zlib_inflate code collects the total number of bytes decompressed over
the multiple calls into the stream.total_out field.
There is clearly no such field available in the cryto API, leading
to the somewhat clumsy need to track it, i.e. it leads to the following
additional code.
+ unsigned int produced = 0;
[snip]
> + length = 0;
[snip]
> + produced = req.avail_out;
> + error = crypto_decompress_update(msblk->tfm, &req);
> + produced -= req.avail_out;
>
> + length += produced;
>
[snip]
> + produced = req.avail_out;
> + error = crypto_decompress_final(msblk->tfm, &req);
> + produced -= req.avail_out;
> +
> + length += produced;
Whereas previously, only the following single line was required:
> - length = msblk->stream.total_out;
2. Major issue - working out loop termination.
It transpires when decompressing from multiple input buffers into
multiple output buffers, determining when the decompressor has consumed
all input buffers and has flushed all output to the output buffers is
difficult.
One might assume loop termination can take place once the decompressor
has consumed all the input data - however, this is insufficient because
the decompressor may exit having consumed all the input data but it
still requires additional output buffer space (stream.avail_in == 0,
stream.avail_out == 0).
This leads to the exit condition (stream.avail_in == 0 &&
stream.avail_out != 0). However, this still isn't sufficient because
the majority of blocks in Squashfs decompress to an exact multiple of
the output buffer, i.e. stream.avail_in == 0 && stream.avail_out == 0 is
true at the end of decompression.
(stream.avail_in == 0 && stream_avail_out == 0) == true may or may not
indicate the end of decompression. In other words the status of the
input/output buffers doesn't give sufficient information as to whether
to terminate the loop or not.
With zlib_inflate this is irrelevant because it supplies a suitable exit
code indicating whether it needs to be called again (Z_OK) or whether
decompression has finished (Z_STREAM_END). This makes loop termination
easy.
My biggest criticism against the cryto changes to Squashfs is
crypto_decompress_update doesn't seem to give this information, leaving
to the clumsy introduction of a check to see if crypto_decompress_update
produced any output data, i.e.:
- } while (zlib_err == Z_OK);
> -
is replaced by:
> + } while (bytes || produced);
> +
This is clearly suboptimal, and always leads to an additional iteration
around the loop. Only once we've iterated over the loop one last time
doing nothing do we decide the decompression has completed.
Not only this, but the loop termination also suffers from a major
unanticipated bug:
The loop termination forces an additional iteration around the loop even
though we've run out of output buffer space.
Consider the usual scenario where we're decompressing a buffer into two
4K pages (pages = 2), and the buffer decompresses to 8K. The final
"real" iteration will have produced != 0 and req.avail_out == 0 (we've
consumed all the output bytes in the last output buffer).
Because produced != 0 we will iterate over the loop again. But because
req.avail_out == 0 we will load req.next_out with a non-existent buffer
(we will fall off the end of the buffer array), i.e.
+ if (req.avail_out == 0) {
> + req.next_out = buffer[page++];
> + req.avail_out = PAGE_CACHE_SIZE;
> }
>
If for any reason crypto_decompress_update produces unexpected output
(perhaps because of corrupted data), we will trigger a kernel oops.
Obviously in the majority of cases (which is why the code works), the
"false" additional iteration doesn't produce any output. But it is
distinctly bad practice to have code in the kernel that in normal
operation passes a bad pointer to crypto_decompress_output, and relies
on its behaviour not to use that bad pointer.
> Thanks,
Currently as it stands I cannot accept the patch into Squashfs for the
above reasons. The code is less optimal, suffers from a major bug, and
has a number of workarounds stemming from what I feel is a less
"featureful" API.
I'm willing to consider moving Squashfs over to the crypto API once my
concerns have been addressed. The promised simplification in the
"addition of support for hardware decompression and different
decompression algorithms" seems quite attractive. But I'm unwilling to
degrade the zlib support in Squashfs for this future promise.
Phillip
Hi Phillip,
On Sun, 8 Mar 2009, Phillip Lougher wrote:
> Herbert Xu wrote:
> > On Wed, Feb 25, 2009 at 02:43:14PM +0100, Geert Uytterhoeven wrote:
> > > Modify SquashFS 4 to use the new "pcomp" crypto interface for
> > > decompression,
> > > instead of calling the underlying zlib library directly. This simplifies
> > > e.g.
> > > the addition of support for hardware decompression and different
> > > decompression
> > > algorithms.
> > >
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > Cc: Phillip Lougher <[email protected]>
> >
> > I've applied patches 1-5. I'd like to see an ack on this before
> > applying it.
>
> I've not acked it because I've not yet made any decisions as to whether I want
> it in Squashfs. Also as Squashfs maintainer I maintain my own tree of patches
> to Squashfs, and so I'll prefer to add it to my tree for subsequent feeding
> into 2.6.30 once the merge window opens.
OK.
> I'm fairly agnostic about what decompression library Squashfs uses. The only
> thing I care about is cleanliness and usability of the API and performance.
> If the crypto API is cleaner or the cryto zlib implementation is faster than
> the current zlib implementation then I see no reason not to move over to it.
> But, I've seen no performance figures and the API seems clumsier.
I did not see any noticeable performance impact due to my changes.
> Two API issues of concern (one major, one minor). Both of these relate to the
> way Squashfs drives the decompression code, where it repeatedly calls it
> supplying additional input/output buffers, rather than using a "single-shot"
> approach where it calls the decompression code once supplying all the
> necessary input and output buffer space.
>
> 1. Minor issue -the lack of a stream.total_out field. The current
> zlib_inflate code collects the total number of bytes decompressed over the
> multiple calls into the stream.total_out field.
>
> There is clearly no such field available in the cryto API, leading to the
> somewhat clumsy need to track it, i.e. it leads to the following additional
> code.
If people feel the need for a total_out field, I can add it to struct
comp_request.
BTW, what about total_in, which is also provided by plain zlib's z_stream?
Do people see a need for a similar field?
> 2. Major issue - working out loop termination.
>
> It transpires when decompressing from multiple input buffers into multiple
> output buffers, determining when the decompressor has consumed all input
> buffers and has flushed all output to the output buffers is difficult.
[...]
> With zlib_inflate this is irrelevant because it supplies a suitable exit code
> indicating whether it needs to be called again (Z_OK) or whether decompression
> has finished (Z_STREAM_END). This makes loop termination easy.
Zlib indeed provides such a flag. Other decompression algorithms may not
provide this, and keep on `decompressing' as long as you feed them data.
So while I could add an output flag indicating decompression has finished, it
cannot be more than a mere hint when considering support for multiple
(de)compression algorithms.
> My biggest criticism against the cryto changes to Squashfs is
> crypto_decompress_update doesn't seem to give this information, leaving to the
> clumsy introduction of a check to see if crypto_decompress_update produced any
> output data, i.e.:
>
> - } while (zlib_err == Z_OK);
> > -
>
> is replaced by:
>
> > + } while (bytes || produced);
> > +
>
> This is clearly suboptimal, and always leads to an additional iteration around
> the loop. Only once we've iterated over the loop one last time doing nothing
> do we decide the decompression has completed.
Given the difficulty in determining the finishing of decompression in a generic
way, I don't see a better solution. If no data has been consumed nor produced,
and no -EAGAIN is returned, decompression is finished.
BTW, this is also very similar to reading from a file: read() returns a
non-zero count until the end of the file is reached.
The additional loop also doesn't seem to have any noticeable impact on
performance, though.
> Not only this, but the loop termination also suffers from a major
> unanticipated bug:
>
> The loop termination forces an additional iteration around the loop even
> though we've run out of output buffer space.
>
> Consider the usual scenario where we're decompressing a buffer into two 4K
> pages (pages = 2), and the buffer decompresses to 8K. The final "real"
> iteration will have produced != 0 and req.avail_out == 0 (we've consumed all
> the output bytes in the last output buffer).
>
> Because produced != 0 we will iterate over the loop again. But because
> req.avail_out == 0 we will load req.next_out with a non-existent buffer (we
> will fall off the end of the buffer array), i.e.
>
> + if (req.avail_out == 0) {
> > + req.next_out = buffer[page++];
> > + req.avail_out = PAGE_CACHE_SIZE;
> > }
> >
>
> If for any reason crypto_decompress_update produces unexpected output (perhaps
> because of corrupted data), we will trigger a kernel oops.
>
> Obviously in the majority of cases (which is why the code works), the "false"
> additional iteration doesn't produce any output. But it is distinctly bad
> practice to have code in the kernel that in normal operation passes a bad
> pointer to crypto_decompress_output, and relies on its behaviour not to use
> that bad pointer.
You are right. Hence this needs a check for page < pages, cfr. your recent fix
to survive corrupted file system images. I'll take care of that.
Thanks for your comments!
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Wed, 11 Mar 2009, Geert Uytterhoeven wrote:
> On Sun, 8 Mar 2009, Phillip Lougher wrote:
> > Two API issues of concern (one major, one minor). Both of these relate to the
> > way Squashfs drives the decompression code, where it repeatedly calls it
> > supplying additional input/output buffers, rather than using a "single-shot"
> > approach where it calls the decompression code once supplying all the
> > necessary input and output buffer space.
> >
> > 1. Minor issue -the lack of a stream.total_out field. The current
> > zlib_inflate code collects the total number of bytes decompressed over the
> > multiple calls into the stream.total_out field.
> >
> > There is clearly no such field available in the cryto API, leading to the
> > somewhat clumsy need to track it, i.e. it leads to the following additional
> > code.
>
> If people feel the need for a total_out field, I can add it to struct
> comp_request.
>
> BTW, what about total_in, which is also provided by plain zlib's z_stream?
> Do people see a need for a similar field?
The patch below (on top of the updated one to convert SquashFS to pcomp) adds
comp_request.total_out, so you don't have to calculate and accumulate the
decompressed output sizes in SquashFS.
Notes:
- This required the addition of a `struct comp_request *' parameter to
crypto_{,de}compress_init()
- Still, there's one of the
produced = req.avail_out;
...
produced -= req.avail_out;
left, as this is part of the logic to discover the end of decompression
(no bytes produced, no error returned).
Perhaps it's better to instead make crypto_{,de}compress_{update,final}()
return the (positive) number of output bytes (of the current step)?
Currently it returns zero (no error) or a negative error value.
That would allow to get rid of both `produced = ... / produced -= ...'
constructs, but the user would have to accumulate the total output size again
(which is not such a big deal, IMHO).
Thanks for your comments!
>From e43f85baa75668be4cce340ae98a3b76e66a452a Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <[email protected]>
Date: Mon, 16 Mar 2009 15:53:30 +0100
Subject: [PATCH] crypto: compress - Add comp_request.total_out
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
crypto/testmgr.c | 4 ++--
crypto/zlib.c | 12 ++++++++++--
fs/squashfs/block.c | 10 +++-------
include/crypto/compress.h | 17 +++++++++++------
4 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index b50c3c6..2b112ae 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -927,7 +927,7 @@ static int test_pcomp(struct crypto_pcomp *tfm,
return error;
}
- error = crypto_compress_init(tfm);
+ error = crypto_compress_init(tfm, &req);
if (error) {
pr_err("alg: pcomp: compression init failed on test "
"%d for %s: error=%d\n", i + 1, algo, error);
@@ -996,7 +996,7 @@ static int test_pcomp(struct crypto_pcomp *tfm,
return error;
}
- error = crypto_decompress_init(tfm);
+ error = crypto_decompress_init(tfm, &req);
if (error) {
pr_err("alg: pcomp: decompression init failed on test "
"%d for %s: error=%d\n", i + 1, algo, error);
diff --git a/crypto/zlib.c b/crypto/zlib.c
index 33609ba..93ec380 100644
--- a/crypto/zlib.c
+++ b/crypto/zlib.c
@@ -125,7 +125,8 @@ static int zlib_compress_setup(struct crypto_pcomp *tfm, void *params,
return 0;
}
-static int zlib_compress_init(struct crypto_pcomp *tfm)
+static int zlib_compress_init(struct crypto_pcomp *tfm,
+ struct comp_request *req)
{
int ret;
struct zlib_ctx *dctx = crypto_tfm_ctx(crypto_pcomp_tfm(tfm));
@@ -135,6 +136,7 @@ static int zlib_compress_init(struct crypto_pcomp *tfm)
if (ret != Z_OK)
return -EINVAL;
+ req->total_out = 0;
return 0;
}
@@ -173,6 +175,7 @@ static int zlib_compress_update(struct crypto_pcomp *tfm,
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
+ req->total_out = stream->total_out;
return 0;
}
@@ -203,6 +206,7 @@ static int zlib_compress_final(struct crypto_pcomp *tfm,
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
+ req->total_out = stream->total_out;
return 0;
}
@@ -239,7 +243,8 @@ static int zlib_decompress_setup(struct crypto_pcomp *tfm, void *params,
return 0;
}
-static int zlib_decompress_init(struct crypto_pcomp *tfm)
+static int zlib_decompress_init(struct crypto_pcomp *tfm,
+ struct comp_request *req)
{
int ret;
struct zlib_ctx *dctx = crypto_tfm_ctx(crypto_pcomp_tfm(tfm));
@@ -249,6 +254,7 @@ static int zlib_decompress_init(struct crypto_pcomp *tfm)
if (ret != Z_OK)
return -EINVAL;
+ req->total_out = 0;
return 0;
}
@@ -288,6 +294,7 @@ static int zlib_decompress_update(struct crypto_pcomp *tfm,
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
+ req->total_out = stream->total_out;
return 0;
}
@@ -336,6 +343,7 @@ static int zlib_decompress_final(struct crypto_pcomp *tfm,
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
+ req->total_out = stream->total_out;
return 0;
}
diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index 6196821..11e19b6 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -168,7 +168,6 @@ int squashfs_read_data(struct super_block *sb, void **buffer, u64 index,
req.avail_in = 0;
bytes = length;
- length = 0;
do {
if (req.avail_in == 0 && k < b) {
avail = min(bytes, msblk->devblksize - offset);
@@ -194,7 +193,8 @@ int squashfs_read_data(struct super_block *sb, void **buffer, u64 index,
}
if (!decomp_init) {
- error = crypto_decompress_init(msblk->tfm);
+ error = crypto_decompress_init(msblk->tfm,
+ &req);
if (error) {
ERROR("crypto_decompress_init "
"returned %d, srclength %d\n",
@@ -213,22 +213,18 @@ int squashfs_read_data(struct super_block *sb, void **buffer, u64 index,
}
produced -= req.avail_out;
- length += produced;
-
if (req.avail_in == 0 && k < b)
put_bh(bh[k++]);
} while (bytes || produced);
- produced = req.avail_out;
error = crypto_decompress_final(msblk->tfm, &req);
if (error) {
ERROR("crypto_decompress_final returned %d, data "
"probably corrupt\n", error);
goto release_mutex;
}
- produced -= req.avail_out;
- length += produced;
+ length = req.total_out;
mutex_unlock(&msblk->read_data_mutex);
} else {
diff --git a/include/crypto/compress.h b/include/crypto/compress.h
index 86163ef..d872c06 100644
--- a/include/crypto/compress.h
+++ b/include/crypto/compress.h
@@ -28,6 +28,7 @@ struct comp_request {
void *next_out; /* next output byte */
unsigned int avail_in; /* bytes available at next_in */
unsigned int avail_out; /* bytes available at next_out */
+ size_t total_out; /* total bytes output so far */
};
enum zlib_comp_params {
@@ -57,14 +58,16 @@ struct crypto_pcomp {
struct pcomp_alg {
int (*compress_setup)(struct crypto_pcomp *tfm, void *params,
unsigned int len);
- int (*compress_init)(struct crypto_pcomp *tfm);
+ int (*compress_init)(struct crypto_pcomp *tfm,
+ struct comp_request *req);
int (*compress_update)(struct crypto_pcomp *tfm,
struct comp_request *req);
int (*compress_final)(struct crypto_pcomp *tfm,
struct comp_request *req);
int (*decompress_setup)(struct crypto_pcomp *tfm, void *params,
unsigned int len);
- int (*decompress_init)(struct crypto_pcomp *tfm);
+ int (*decompress_init)(struct crypto_pcomp *tfm,
+ struct comp_request *req);
int (*decompress_update)(struct crypto_pcomp *tfm,
struct comp_request *req);
int (*decompress_final)(struct crypto_pcomp *tfm,
@@ -102,9 +105,10 @@ static inline int crypto_compress_setup(struct crypto_pcomp *tfm,
return crypto_pcomp_alg(tfm)->compress_setup(tfm, params, len);
}
-static inline int crypto_compress_init(struct crypto_pcomp *tfm)
+static inline int crypto_compress_init(struct crypto_pcomp *tfm,
+ struct comp_request *req)
{
- return crypto_pcomp_alg(tfm)->compress_init(tfm);
+ return crypto_pcomp_alg(tfm)->compress_init(tfm, req);
}
static inline int crypto_compress_update(struct crypto_pcomp *tfm,
@@ -125,9 +129,10 @@ static inline int crypto_decompress_setup(struct crypto_pcomp *tfm,
return crypto_pcomp_alg(tfm)->decompress_setup(tfm, params, len);
}
-static inline int crypto_decompress_init(struct crypto_pcomp *tfm)
+static inline int crypto_decompress_init(struct crypto_pcomp *tfm,
+ struct comp_request *req)
{
- return crypto_pcomp_alg(tfm)->decompress_init(tfm);
+ return crypto_pcomp_alg(tfm)->decompress_init(tfm, req);
}
static inline int crypto_decompress_update(struct crypto_pcomp *tfm,
--
1.6.0.4
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
On Tue, 17 Mar 2009, Geert Uytterhoeven wrote:
> On Wed, 11 Mar 2009, Geert Uytterhoeven wrote:
> > On Sun, 8 Mar 2009, Phillip Lougher wrote:
> > > Two API issues of concern (one major, one minor). Both of these relate to the
> > > way Squashfs drives the decompression code, where it repeatedly calls it
> > > supplying additional input/output buffers, rather than using a "single-shot"
> > > approach where it calls the decompression code once supplying all the
> > > necessary input and output buffer space.
> > >
> > > 1. Minor issue -the lack of a stream.total_out field. The current
> > > zlib_inflate code collects the total number of bytes decompressed over the
> > > multiple calls into the stream.total_out field.
> > >
> > > There is clearly no such field available in the cryto API, leading to the
> > > somewhat clumsy need to track it, i.e. it leads to the following additional
> > > code.
> >
> > If people feel the need for a total_out field, I can add it to struct
> > comp_request.
> >
> > BTW, what about total_in, which is also provided by plain zlib's z_stream?
> > Do people see a need for a similar field?
>
> The patch below (on top of the updated one to convert SquashFS to pcomp) adds
> comp_request.total_out, so you don't have to calculate and accumulate the
> decompressed output sizes in SquashFS.
>
> Notes:
> - This required the addition of a `struct comp_request *' parameter to
> crypto_{,de}compress_init()
> - Still, there's one of the
>
> produced = req.avail_out;
> ...
> produced -= req.avail_out;
>
> left, as this is part of the logic to discover the end of decompression
> (no bytes produced, no error returned).
>
> Perhaps it's better to instead make crypto_{,de}compress_{update,final}()
> return the (positive) number of output bytes (of the current step)?
>
> Currently it returns zero (no error) or a negative error value.
> That would allow to get rid of both `produced = ... / produced -= ...'
> constructs, but the user would have to accumulate the total output size again
> (which is not such a big deal, IMHO).
Here's an alternative patch, which does exactly that.
Phillip, what do you think?
Thanks for your comments!
>From be7d630f96a85d3ce48716b8e328563ba217647b Mon Sep 17 00:00:00 2001
From: Geert Uytterhoeven <[email protected]>
Date: Tue, 24 Mar 2009 17:19:05 +0100
Subject: [PATCH] crypto: compress - Return produced bytes in crypto_{,de}compress_{update,final}()
If crypto_{,de}compress_{update,final}() succeed, return the actual number of
bytes produced instead of zero, so their users don't have to calculate that
theirselves.
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
crypto/testmgr.c | 117 ++++++++++++++++++++++++++++++--------------------
crypto/zlib.c | 24 +++++-----
fs/squashfs/block.c | 33 ++++++---------
3 files changed, 95 insertions(+), 79 deletions(-)
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index b50c3c6..9cee018 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -914,24 +914,25 @@ static int test_pcomp(struct crypto_pcomp *tfm,
const char *algo = crypto_tfm_alg_driver_name(crypto_pcomp_tfm(tfm));
unsigned int i;
char result[COMP_BUF_SIZE];
- int error;
+ int res;
for (i = 0; i < ctcount; i++) {
struct comp_request req;
+ unsigned int produced = 0;
- error = crypto_compress_setup(tfm, ctemplate[i].params,
- ctemplate[i].paramsize);
- if (error) {
+ res = crypto_compress_setup(tfm, ctemplate[i].params,
+ ctemplate[i].paramsize);
+ if (res) {
pr_err("alg: pcomp: compression setup failed on test "
- "%d for %s: error=%d\n", i + 1, algo, error);
- return error;
+ "%d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
- error = crypto_compress_init(tfm);
- if (error) {
+ res = crypto_compress_init(tfm);
+ if (res) {
pr_err("alg: pcomp: compression init failed on test "
- "%d for %s: error=%d\n", i + 1, algo, error);
- return error;
+ "%d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
memset(result, 0, sizeof(result));
@@ -941,32 +942,37 @@ static int test_pcomp(struct crypto_pcomp *tfm,
req.next_out = result;
req.avail_out = ctemplate[i].outlen / 2;
- error = crypto_compress_update(tfm, &req);
- if (error && (error != -EAGAIN || req.avail_in)) {
+ res = crypto_compress_update(tfm, &req);
+ if (res < 0 && (res != -EAGAIN || req.avail_in)) {
pr_err("alg: pcomp: compression update failed on test "
- "%d for %s: error=%d\n", i + 1, algo, error);
- return error;
+ "%d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
+ if (res > 0)
+ produced += res;
/* Add remaining input data */
req.avail_in += (ctemplate[i].inlen + 1) / 2;
- error = crypto_compress_update(tfm, &req);
- if (error && (error != -EAGAIN || req.avail_in)) {
+ res = crypto_compress_update(tfm, &req);
+ if (res < 0 && (res != -EAGAIN || req.avail_in)) {
pr_err("alg: pcomp: compression update failed on test "
- "%d for %s: error=%d\n", i + 1, algo, error);
- return error;
+ "%d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
+ if (res > 0)
+ produced += res;
/* Provide remaining output space */
req.avail_out += COMP_BUF_SIZE - ctemplate[i].outlen / 2;
- error = crypto_compress_final(tfm, &req);
- if (error) {
+ res = crypto_compress_final(tfm, &req);
+ if (res < 0) {
pr_err("alg: pcomp: compression final failed on test "
- "%d for %s: error=%d\n", i + 1, algo, error);
- return error;
+ "%d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
+ produced += res;
if (COMP_BUF_SIZE - req.avail_out != ctemplate[i].outlen) {
pr_err("alg: comp: Compression test %d failed for %s: "
@@ -976,6 +982,13 @@ static int test_pcomp(struct crypto_pcomp *tfm,
return -EINVAL;
}
+ if (produced != ctemplate[i].outlen) {
+ pr_err("alg: comp: Compression test %d failed for %s: "
+ "returned len = %u (expected %d)\n", i + 1,
+ algo, produced, ctemplate[i].outlen);
+ return -EINVAL;
+ }
+
if (memcmp(result, ctemplate[i].output, ctemplate[i].outlen)) {
pr_err("alg: pcomp: Compression test %d failed for "
"%s\n", i + 1, algo);
@@ -986,21 +999,21 @@ static int test_pcomp(struct crypto_pcomp *tfm,
for (i = 0; i < dtcount; i++) {
struct comp_request req;
+ unsigned int produced = 0;
- error = crypto_decompress_setup(tfm, dtemplate[i].params,
- dtemplate[i].paramsize);
- if (error) {
+ res = crypto_decompress_setup(tfm, dtemplate[i].params,
+ dtemplate[i].paramsize);
+ if (res) {
pr_err("alg: pcomp: decompression setup failed on "
- "test %d for %s: error=%d\n", i + 1, algo,
- error);
- return error;
+ "test %d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
- error = crypto_decompress_init(tfm);
- if (error) {
+ res = crypto_decompress_init(tfm);
+ if (res) {
pr_err("alg: pcomp: decompression init failed on test "
- "%d for %s: error=%d\n", i + 1, algo, error);
- return error;
+ "%d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
memset(result, 0, sizeof(result));
@@ -1010,35 +1023,38 @@ static int test_pcomp(struct crypto_pcomp *tfm,
req.next_out = result;
req.avail_out = dtemplate[i].outlen / 2;
- error = crypto_decompress_update(tfm, &req);
- if (error && (error != -EAGAIN || req.avail_in)) {
+ res = crypto_decompress_update(tfm, &req);
+ if (res < 0 && (res != -EAGAIN || req.avail_in)) {
pr_err("alg: pcomp: decompression update failed on "
- "test %d for %s: error=%d\n", i + 1, algo,
- error);
- return error;
+ "test %d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
+ if (res > 0)
+ produced += res;
/* Add remaining input data */
req.avail_in += (dtemplate[i].inlen + 1) / 2;
- error = crypto_decompress_update(tfm, &req);
- if (error && (error != -EAGAIN || req.avail_in)) {
+ res = crypto_decompress_update(tfm, &req);
+ if (res < 0 && (res != -EAGAIN || req.avail_in)) {
pr_err("alg: pcomp: decompression update failed on "
- "test %d for %s: error=%d\n", i + 1, algo,
- error);
- return error;
+ "test %d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
+ if (res > 0)
+ produced += res;
/* Provide remaining output space */
req.avail_out += COMP_BUF_SIZE - dtemplate[i].outlen / 2;
- error = crypto_decompress_final(tfm, &req);
- if (error && (error != -EAGAIN || req.avail_in)) {
+ res = crypto_decompress_final(tfm, &req);
+ if (res < 0 && (res != -EAGAIN || req.avail_in)) {
pr_err("alg: pcomp: decompression final failed on "
- "test %d for %s: error=%d\n", i + 1, algo,
- error);
- return error;
+ "test %d for %s: error=%d\n", i + 1, algo, res);
+ return res;
}
+ if (res > 0)
+ produced += res;
if (COMP_BUF_SIZE - req.avail_out != dtemplate[i].outlen) {
pr_err("alg: comp: Decompression test %d failed for "
@@ -1048,6 +1064,13 @@ static int test_pcomp(struct crypto_pcomp *tfm,
return -EINVAL;
}
+ if (produced != dtemplate[i].outlen) {
+ pr_err("alg: comp: Decompression test %d failed for "
+ "%s: returned len = %u (expected %d)\n", i + 1,
+ algo, produced, dtemplate[i].outlen);
+ return -EINVAL;
+ }
+
if (memcmp(result, dtemplate[i].output, dtemplate[i].outlen)) {
pr_err("alg: pcomp: Decompression test %d failed for "
"%s\n", i + 1, algo);
diff --git a/crypto/zlib.c b/crypto/zlib.c
index 33609ba..c301573 100644
--- a/crypto/zlib.c
+++ b/crypto/zlib.c
@@ -165,15 +165,15 @@ static int zlib_compress_update(struct crypto_pcomp *tfm,
return -EINVAL;
}
+ ret = req->avail_out - stream->avail_out;
pr_debug("avail_in %u, avail_out %u (consumed %u, produced %u)\n",
stream->avail_in, stream->avail_out,
- req->avail_in - stream->avail_in,
- req->avail_out - stream->avail_out);
+ req->avail_in - stream->avail_in, ret);
req->next_in = stream->next_in;
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
- return 0;
+ return ret;
}
static int zlib_compress_final(struct crypto_pcomp *tfm,
@@ -195,15 +195,15 @@ static int zlib_compress_final(struct crypto_pcomp *tfm,
return -EINVAL;
}
+ ret = req->avail_out - stream->avail_out;
pr_debug("avail_in %u, avail_out %u (consumed %u, produced %u)\n",
stream->avail_in, stream->avail_out,
- req->avail_in - stream->avail_in,
- req->avail_out - stream->avail_out);
+ req->avail_in - stream->avail_in, ret);
req->next_in = stream->next_in;
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
- return 0;
+ return ret;
}
@@ -280,15 +280,15 @@ static int zlib_decompress_update(struct crypto_pcomp *tfm,
return -EINVAL;
}
+ ret = req->avail_out - stream->avail_out;
pr_debug("avail_in %u, avail_out %u (consumed %u, produced %u)\n",
stream->avail_in, stream->avail_out,
- req->avail_in - stream->avail_in,
- req->avail_out - stream->avail_out);
+ req->avail_in - stream->avail_in, ret);
req->next_in = stream->next_in;
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
- return 0;
+ return ret;
}
static int zlib_decompress_final(struct crypto_pcomp *tfm,
@@ -328,15 +328,15 @@ static int zlib_decompress_final(struct crypto_pcomp *tfm,
return -EINVAL;
}
+ ret = req->avail_out - stream->avail_out;
pr_debug("avail_in %u, avail_out %u (consumed %u, produced %u)\n",
stream->avail_in, stream->avail_out,
- req->avail_in - stream->avail_in,
- req->avail_out - stream->avail_out);
+ req->avail_in - stream->avail_in, ret);
req->next_in = stream->next_in;
req->avail_in = stream->avail_in;
req->next_out = stream->next_out;
req->avail_out = stream->avail_out;
- return 0;
+ return ret;
}
diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index 6196821..433f065 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -154,9 +154,8 @@ int squashfs_read_data(struct super_block *sb, void **buffer, u64 index,
}
if (compressed) {
- int error = 0, decomp_init = 0;
+ int res = 0, decomp_init = 0;
struct comp_request req;
- unsigned int produced = 0;
/*
* Uncompress block.
@@ -194,41 +193,35 @@ int squashfs_read_data(struct super_block *sb, void **buffer, u64 index,
}
if (!decomp_init) {
- error = crypto_decompress_init(msblk->tfm);
- if (error) {
+ res = crypto_decompress_init(msblk->tfm);
+ if (res) {
ERROR("crypto_decompress_init "
"returned %d, srclength %d\n",
- error, srclength);
+ res, srclength);
goto release_mutex;
}
decomp_init = 1;
}
- produced = req.avail_out;
- error = crypto_decompress_update(msblk->tfm, &req);
- if (error) {
+ res = crypto_decompress_update(msblk->tfm, &req);
+ if (res < 0) {
ERROR("crypto_decompress_update returned %d, "
- "data probably corrupt\n", error);
+ "data probably corrupt\n", res);
goto release_mutex;
}
- produced -= req.avail_out;
-
- length += produced;
+ length += res;
if (req.avail_in == 0 && k < b)
put_bh(bh[k++]);
- } while (bytes || produced);
+ } while (bytes || res);
- produced = req.avail_out;
- error = crypto_decompress_final(msblk->tfm, &req);
- if (error) {
+ res = crypto_decompress_final(msblk->tfm, &req);
+ if (res < 0) {
ERROR("crypto_decompress_final returned %d, data "
- "probably corrupt\n", error);
+ "probably corrupt\n", res);
goto release_mutex;
}
- produced -= req.avail_out;
-
- length += produced;
+ length += res;
mutex_unlock(&msblk->read_data_mutex);
} else {
--
1.6.0.4
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
Geert Uytterhoeven wrote:
> On Tue, 17 Mar 2009, Geert Uytterhoeven wrote:
>> On Wed, 11 Mar 2009, Geert Uytterhoeven wrote:
>>> On Sun, 8 Mar 2009, Phillip Lougher wrote:
>>>> Two API issues of concern (one major, one minor). Both of these relate to the
>>>> way Squashfs drives the decompression code, where it repeatedly calls it
>>>> supplying additional input/output buffers, rather than using a "single-shot"
>>>> approach where it calls the decompression code once supplying all the
>>>> necessary input and output buffer space.
>>>>
>>>> 1. Minor issue -the lack of a stream.total_out field. The current
>>>> zlib_inflate code collects the total number of bytes decompressed over the
>>>> multiple calls into the stream.total_out field.
>>>>
>>>> There is clearly no such field available in the cryto API, leading to the
>>>> somewhat clumsy need to track it, i.e. it leads to the following additional
>>>> code.
>>> If people feel the need for a total_out field, I can add it to struct
>>> comp_request.
>>>
>>> BTW, what about total_in, which is also provided by plain zlib's z_stream?
>>> Do people see a need for a similar field?
>> The patch below (on top of the updated one to convert SquashFS to pcomp) adds
>> comp_request.total_out, so you don't have to calculate and accumulate the
>> decompressed output sizes in SquashFS.
>>
>> Notes:
>> - This required the addition of a `struct comp_request *' parameter to
>> crypto_{,de}compress_init()
>> - Still, there's one of the
>>
>> produced = req.avail_out;
>> ...
>> produced -= req.avail_out;
>>
>> left, as this is part of the logic to discover the end of decompression
>> (no bytes produced, no error returned).
>>
>> Perhaps it's better to instead make crypto_{,de}compress_{update,final}()
>> return the (positive) number of output bytes (of the current step)?
>>
>> Currently it returns zero (no error) or a negative error value.
>> That would allow to get rid of both `produced = ... / produced -= ...'
>> constructs, but the user would have to accumulate the total output size again
>> (which is not such a big deal, IMHO).
>
> Here's an alternative patch, which does exactly that.
> Phillip, what do you think?
>
From a quick look, it looks OK :-) I'm not ignoring this, but I'm trying to get a release of
the 4.0 tools finished ASAP (now 2.6.29 is out).
Phillip
On Tue, Mar 24, 2009 at 05:33:01PM +0100, Geert Uytterhoeven wrote:
>
> Here's an alternative patch, which does exactly that.
> Phillip, what do you think?
>
> Thanks for your comments!
>
> >From be7d630f96a85d3ce48716b8e328563ba217647b Mon Sep 17 00:00:00 2001
> From: Geert Uytterhoeven <[email protected]>
> Date: Tue, 24 Mar 2009 17:19:05 +0100
> Subject: [PATCH] crypto: compress - Return produced bytes in crypto_{,de}compress_{update,final}()
>
> If crypto_{,de}compress_{update,final}() succeed, return the actual number of
> bytes produced instead of zero, so their users don't have to calculate that
> theirselves.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
I certainly prefer this version over the other one? Do you want to
submit crypto API portion of this?
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, 20 Apr 2009, Herbert Xu wrote:
> On Tue, Mar 24, 2009 at 05:33:01PM +0100, Geert Uytterhoeven wrote:
> > Here's an alternative patch, which does exactly that.
> > Phillip, what do you think?
> >
> > Thanks for your comments!
> >
> > >From be7d630f96a85d3ce48716b8e328563ba217647b Mon Sep 17 00:00:00 2001
> > From: Geert Uytterhoeven <[email protected]>
> > Date: Tue, 24 Mar 2009 17:19:05 +0100
> > Subject: [PATCH] crypto: compress - Return produced bytes in crypto_{,de}compress_{update,final}()
> >
> > If crypto_{,de}compress_{update,final}() succeed, return the actual number of
> > bytes produced instead of zero, so their users don't have to calculate that
> > theirselves.
> >
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> I certainly prefer this version over the other one? Do you want to
> submit crypto API portion of this?
If you think I should submit it now, I can do it. But I'm still waiting for
Phillip's comments.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010
Geert Uytterhoeven wrote:
> On Mon, 20 Apr 2009, Herbert Xu wrote:
>> On Tue, Mar 24, 2009 at 05:33:01PM +0100, Geert Uytterhoeven wrote:
>>> Here's an alternative patch, which does exactly that.
>>> Phillip, what do you think?
>>>
>>> Thanks for your comments!
>>>
>>> >From be7d630f96a85d3ce48716b8e328563ba217647b Mon Sep 17 00:00:00 2001
>>> From: Geert Uytterhoeven <[email protected]>
>>> Date: Tue, 24 Mar 2009 17:19:05 +0100
>>> Subject: [PATCH] crypto: compress - Return produced bytes in crypto_{,de}compress_{update,final}()
>>>
>>> If crypto_{,de}compress_{update,final}() succeed, return the actual number of
>>> bytes produced instead of zero, so their users don't have to calculate that
>>> theirselves.
>>>
>>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> I certainly prefer this version over the other one? Do you want to
>> submit crypto API portion of this?
>
> If you think I should submit it now, I can do it. But I'm still waiting for
> Phillip's comments.
>
I think I said they looked OK to me. But, I want to do performance tests
to see if there's any performance degradation over vanilla zlib, and see
if they cope gracefully with corrupted filesystems.
Herbert, are the other cryto API patches in linux-next (or any other
git repository)?
Thanks
Phillip
On Tue, Apr 21, 2009 at 12:45:19AM +0100, Phillip Lougher wrote:
>
> Herbert, are the other cryto API patches in linux-next (or any other
> git repository)?
They should be in Linus's tree.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, 21 Apr 2009, Phillip Lougher wrote:
> Geert Uytterhoeven wrote:
> > On Mon, 20 Apr 2009, Herbert Xu wrote:
> > > On Tue, Mar 24, 2009 at 05:33:01PM +0100, Geert Uytterhoeven wrote:
> > > > Here's an alternative patch, which does exactly that.
> > > > Phillip, what do you think?
> > > >
> > > > Thanks for your comments!
> > > >
> > > > >From be7d630f96a85d3ce48716b8e328563ba217647b Mon Sep 17 00:00:00 2001
> > > > From: Geert Uytterhoeven <[email protected]>
> > > > Date: Tue, 24 Mar 2009 17:19:05 +0100
> > > > Subject: [PATCH] crypto: compress - Return produced bytes in
> > > > crypto_{,de}compress_{update,final}()
> > > >
> > > > If crypto_{,de}compress_{update,final}() succeed, return the actual
> > > > number of
> > > > bytes produced instead of zero, so their users don't have to calculate
> > > > that
> > > > theirselves.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > I certainly prefer this version over the other one? Do you want to
> > > submit crypto API portion of this?
> >
> > If you think I should submit it now, I can do it. But I'm still waiting for
> > Phillip's comments.
>
> I think I said they looked OK to me. But, I want to do performance tests
> to see if there's any performance degradation over vanilla zlib, and see
> if they cope gracefully with corrupted filesystems.
>
> Herbert, are the other cryto API patches in linux-next (or any other
> git repository)?
(in the mean time, all of this has been in mainline since a while)
Phillip, any news from you?
Thanks!
With kind regards,
Geert Uytterhoeven
Software Architect
Techsoft Centre
Technology and Software Centre Europe
The Corporate Village ? Da Vincilaan 7-D1 ? B-1935 Zaventem ? Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 ? RPR Brussels
Fortis ? BIC GEBABEBB ? IBAN BE41293037680010