Jan Beulich
2011-Mar-09 11:12 UTC
[Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
Signed-off-by: Jan Beulich <jbeulich@novell.com>
--- a/tools/libxc/xc_dom_bzimageloader.c
+++ b/tools/libxc/xc_dom_bzimageloader.c
@@ -140,11 +140,10 @@ static int xc_try_bzip2_decode(
#include <lzma.h>
-static int xc_try_lzma_decode(
- struct xc_dom_image *dom, void **blob, size_t *size)
+static int _xc_try_lzma_decode(
+ struct xc_dom_image *dom, void **blob, size_t *size,
+ lzma_stream *stream, lzma_ret ret, const char *what)
{
- lzma_stream stream = LZMA_STREAM_INIT;
- lzma_ret ret;
lzma_action action = LZMA_RUN;
unsigned char *out_buf;
unsigned char *tmp_buf;
@@ -152,10 +151,9 @@ static int xc_try_lzma_decode(
int outsize;
const char *msg;
- ret = lzma_alone_decoder(&stream, 128*1024*1024);
if ( ret != LZMA_OK )
{
- DOMPRINTF("LZMA: Failed to init stream decoder");
+ DOMPRINTF("%s: Failed to init decoder", what);
return -1;
}
@@ -167,32 +165,32 @@ static int xc_try_lzma_decode(
out_buf = malloc(outsize);
if ( out_buf == NULL )
{
- DOMPRINTF("LZMA: Failed to alloc memory");
+ DOMPRINTF("%s: Failed to alloc memory", what);
goto lzma_cleanup;
}
- stream.next_in = dom->kernel_blob;
- stream.avail_in = dom->kernel_size;
+ stream->next_in = dom->kernel_blob;
+ stream->avail_in = dom->kernel_size;
- stream.next_out = out_buf;
- stream.avail_out = dom->kernel_size;
+ stream->next_out = out_buf;
+ stream->avail_out = dom->kernel_size;
for ( ; ; )
{
- ret = lzma_code(&stream, action);
- if ( (stream.avail_out == 0) || (ret != LZMA_OK) )
+ ret = lzma_code(stream, action);
+ if ( (stream->avail_out == 0) || (ret != LZMA_OK) )
{
tmp_buf = realloc(out_buf, outsize * 2);
if ( tmp_buf == NULL )
{
- DOMPRINTF("LZMA: Failed to realloc memory");
+ DOMPRINTF("%s: Failed to realloc memory", what);
free(out_buf);
goto lzma_cleanup;
}
out_buf = tmp_buf;
- stream.next_out = out_buf + outsize;
- stream.avail_out = (outsize * 2) - outsize;
+ stream->next_out = out_buf + outsize;
+ stream->avail_out = (outsize * 2) - outsize;
outsize *= 2;
}
@@ -200,7 +198,7 @@ static int xc_try_lzma_decode(
{
if ( ret == LZMA_STREAM_END )
{
- DOMPRINTF("LZMA: Saw data stream end");
+ DOMPRINTF("%s: Saw data stream end", what);
retval = 0;
break;
}
@@ -236,26 +234,55 @@ static int xc_try_lzma_decode(
msg = "Internal program error (bug)";
break;
}
- DOMPRINTF("%s: LZMA decompression error %s",
- __FUNCTION__, msg);
+ DOMPRINTF("%s: %s decompression error: %s",
+ __FUNCTION__, what, msg);
break;
}
}
- DOMPRINTF("%s: LZMA decompress OK, 0x%zx -> 0x%zx",
- __FUNCTION__, *size, (size_t)stream.total_out);
+ DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
+ __FUNCTION__, what, *size, (size_t)stream->total_out);
*blob = out_buf;
- *size = stream.total_out;
+ *size = stream->total_out;
lzma_cleanup:
- lzma_end(&stream);
+ lzma_end(stream);
return retval;
}
+/* 128 Mb is the minimum size (half-way) documented to work for all inputs. */
+#define LZMA_BLOCK_SIZE (128*1024*1024)
+
+static int xc_try_xz_decode(
+ struct xc_dom_image *dom, void **blob, size_t *size)
+{
+ lzma_stream stream = LZMA_STREAM_INIT;
+ lzma_ret ret = lzma_stream_decoder(&stream, LZMA_BLOCK_SIZE, 0);
+
+ return _xc_try_lzma_decode(dom, blob, size, &stream, ret,
"XZ");
+}
+
+static int xc_try_lzma_decode(
+ struct xc_dom_image *dom, void **blob, size_t *size)
+{
+ lzma_stream stream = LZMA_STREAM_INIT;
+ lzma_ret ret = lzma_alone_decoder(&stream, LZMA_BLOCK_SIZE);
+
+ return _xc_try_lzma_decode(dom, blob, size, &stream, ret,
"LZMA");
+}
+
#else /* !defined(HAVE_LZMA) */
+static int xc_try_xz_decode(
+ struct xc_dom_image *dom, void **blob, size_t *size)
+{
+ DOMPRINTF("%s: XZ decompress support unavailable",
+ __FUNCTION__);
+ return -1;
+}
+
static int xc_try_lzma_decode(
struct xc_dom_image *dom, void **blob, size_t *size)
{
@@ -557,6 +584,17 @@ static int xc_dom_probe_bzimage_kernel(s
return -EINVAL;
}
}
+ else if ( memcmp(dom->kernel_blob, "\3757zXZ", 6) == 0 )
+ {
+ ret = xc_try_xz_decode(dom, &dom->kernel_blob,
&dom->kernel_size);
+ if ( ret < 0 )
+ {
+ xc_dom_panic(dom->xch, XC_INVALID_KERNEL,
+ "%s unable to XZ decompress kernel",
+ __FUNCTION__);
+ return -EINVAL;
+ }
+ }
else if ( memcmp(dom->kernel_blob, "\135\000", 2) == 0 )
{
ret = xc_try_lzma_decode(dom, &dom->kernel_blob,
&dom->kernel_size);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-10 17:51 UTC
Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
Jan Beulich writes ("[Xen-devel] [PATCH 2/3] add DomU xz kernel
decompression"):> Signed-off-by: Jan Beulich <jbeulich@novell.com>
I see this has already been committed, but:
> --- a/tools/libxc/xc_dom_bzimageloader.c
> +++ b/tools/libxc/xc_dom_bzimageloader.c
...> {
> - lzma_stream stream = LZMA_STREAM_INIT;
> - lzma_ret ret;
> lzma_action action = LZMA_RUN;
> unsigned char *out_buf;
> unsigned char *tmp_buf;
> @@ -152,10 +151,9 @@ static int xc_try_lzma_decode(
> int outsize;
> const char *msg;
>
> - ret = lzma_alone_decoder(&stream, 128*1024*1024);
> if ( ret != LZMA_OK )
> {
I don''t think this can possibly be correct.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-10 20:17 UTC
Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
>>> Ian Jackson 03/10/11 6:51 PM >>> >Jan Beulich writes ("[Xen-devel] [PATCH 2/3] add DomU xz kernel decompression"): >> Signed-off-by: Jan Beulich > >I see this has already been committed, but: > >> --- a/tools/libxc/xc_dom_bzimageloader.c >> +++ b/tools/libxc/xc_dom_bzimageloader.c >... >> { >> - lzma_stream stream = LZMA_STREAM_INIT; >> - lzma_ret ret; >> lzma_action action = LZMA_RUN; >> unsigned char *out_buf; >> unsigned char *tmp_buf; >> @@ -152,10 +151,9 @@ static int xc_try_lzma_decode( >> int outsize; >> const char *msg; >> >> - ret = lzma_alone_decoder(&stream, 128*1024*1024); >> if ( ret != LZMA_OK ) >> { > >I don''t think this can possibly be correct.At the first glance it may look odd, I agree. However, I tested it and it did work for me. The fact is that "ret" is now getting passed in by the caller, and the invocation of (in this case) lzma_alone_decoder() was moved into the (new) caller. If it''s not that aspect of the change, I may need some more explanation from you as to what you think is wrong. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-11 07:28 UTC
Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
On Thu, 2011-03-10 at 20:17 +0000, Jan Beulich wrote:> >>> Ian Jackson 03/10/11 6:51 PM >>> > >Jan Beulich writes ("[Xen-devel] [PATCH 2/3] add DomU xz kernel decompression"): > >> Signed-off-by: Jan Beulich > > > >I see this has already been committed, but: > > > >> --- a/tools/libxc/xc_dom_bzimageloader.c > >> +++ b/tools/libxc/xc_dom_bzimageloader.c > >... > >> { > >> - lzma_stream stream = LZMA_STREAM_INIT; > >> - lzma_ret ret; > >> lzma_action action = LZMA_RUN; > >> unsigned char *out_buf; > >> unsigned char *tmp_buf; > >> @@ -152,10 +151,9 @@ static int xc_try_lzma_decode( > >> int outsize; > >> const char *msg; > >> > >> - ret = lzma_alone_decoder(&stream, 128*1024*1024); > >> if ( ret != LZMA_OK ) > >> { > > > >I don''t think this can possibly be correct. > > At the first glance it may look odd, I agree. However, I tested it > and it did work for me. The fact is that "ret" is now getting passed > in by the caller, and the invocation of (in this case) lzma_alone_decoder() > was moved into the (new) caller. > > If it''s not that aspect of the change, I may need some more > explanation from you as to what you think is wrong.At the very least the variable is now horribly misnamed. But more importantly I think it''s horribly convoluted, confusing and unexpected to pass the return code of a function called in one function down the callstack into the next (_xc_try_lzma_decode) simply so that function can, as it''s first action, check for failure and return. That check very clearly belongs in each of the callers, right after the failed function call. Ian.> > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Mar-11 08:20 UTC
Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
>>> On 11.03.11 at 08:28, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Thu, 2011-03-10 at 20:17 +0000, Jan Beulich wrote: >> >>> Ian Jackson 03/10/11 6:51 PM >>> >> >Jan Beulich writes ("[Xen-devel] [PATCH 2/3] add DomU xz kernel > decompression"): >> >> Signed-off-by: Jan Beulich >> > >> >I see this has already been committed, but: >> > >> >> --- a/tools/libxc/xc_dom_bzimageloader.c >> >> +++ b/tools/libxc/xc_dom_bzimageloader.c >> >... >> >> { >> >> - lzma_stream stream = LZMA_STREAM_INIT; >> >> - lzma_ret ret; >> >> lzma_action action = LZMA_RUN; >> >> unsigned char *out_buf; >> >> unsigned char *tmp_buf; >> >> @@ -152,10 +151,9 @@ static int xc_try_lzma_decode( >> >> int outsize; >> >> const char *msg; >> >> >> >> - ret = lzma_alone_decoder(&stream, 128*1024*1024); >> >> if ( ret != LZMA_OK ) >> >> { >> > >> >I don''t think this can possibly be correct. >> >> At the first glance it may look odd, I agree. However, I tested it >> and it did work for me. The fact is that "ret" is now getting passed >> in by the caller, and the invocation of (in this case) lzma_alone_decoder() >> was moved into the (new) caller. >> >> If it''s not that aspect of the change, I may need some more >> explanation from you as to what you think is wrong. > > At the very least the variable is now horribly misnamed.I don''t think so - it *is* the return code (and used this way throughout the function).> But more importantly I think it''s horribly convoluted, confusing and > unexpected to pass the return code of a function called in one function > down the callstack into the next (_xc_try_lzma_decode) simply so that > function can, as it''s first action, check for failure and return. > > That check very clearly belongs in each of the callers, right after the > failed function call.That''s a matter of taste, I''d say - I''m favoring the avoidance of code duplication. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Mar-11 08:57 UTC
Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
On Fri, 2011-03-11 at 08:20 +0000, Jan Beulich wrote:> >>> On 11.03.11 at 08:28, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Thu, 2011-03-10 at 20:17 +0000, Jan Beulich wrote: > >> >>> Ian Jackson 03/10/11 6:51 PM >>> > >> >Jan Beulich writes ("[Xen-devel] [PATCH 2/3] add DomU xz kernel > > decompression"): > >> >> Signed-off-by: Jan Beulich > >> > > >> >I see this has already been committed, but: > >> > > >> >> --- a/tools/libxc/xc_dom_bzimageloader.c > >> >> +++ b/tools/libxc/xc_dom_bzimageloader.c > >> >... > >> >> { > >> >> - lzma_stream stream = LZMA_STREAM_INIT; > >> >> - lzma_ret ret; > >> >> lzma_action action = LZMA_RUN; > >> >> unsigned char *out_buf; > >> >> unsigned char *tmp_buf; > >> >> @@ -152,10 +151,9 @@ static int xc_try_lzma_decode( > >> >> int outsize; > >> >> const char *msg; > >> >> > >> >> - ret = lzma_alone_decoder(&stream, 128*1024*1024); > >> >> if ( ret != LZMA_OK ) > >> >> { > >> > > >> >I don''t think this can possibly be correct. > >> > >> At the first glance it may look odd, I agree. However, I tested it > >> and it did work for me. The fact is that "ret" is now getting passed > >> in by the caller, and the invocation of (in this case) lzma_alone_decoder() > >> was moved into the (new) caller. > >> > >> If it''s not that aspect of the change, I may need some more > >> explanation from you as to what you think is wrong. > > > > At the very least the variable is now horribly misnamed. > > I don''t think so - it *is* the return code (and used this way > throughout the function).But in the first instance it is not -- it is only a function parameter in that case.> > But more importantly I think it''s horribly convoluted, confusing and > > unexpected to pass the return code of a function called in one function > > down the callstack into the next (_xc_try_lzma_decode) simply so that > > function can, as it''s first action, check for failure and return. > > > > That check very clearly belongs in each of the callers, right after the > > failed function call. > > That''s a matter of taste, I''d say - I''m favoring the avoidance of > code duplication.By moving an error test from the obvious location next to the function which returned an error down into a subsequent function? To save what, 4 lines of code? Moving the error handling more than 100 lines away from the actual site of the error? I think you''ve taken avoidance of duplication to its illogical extreme here and it is detrimental to the readability and maintainability of the code. 8<------------------------------ # HG changeset patch # User Ian Campbell <ian.campbell@citrix.com> # Date 1299832300 0 # Node ID e6bb5969cdb756ee7b058f0ae23a3c219611f965 # Parent bfd7eeba13dffaa133eca2d2d0814b40b68ffa23 libxc: move error checking next to the function which returned the error. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff -r bfd7eeba13df -r e6bb5969cdb7 tools/libxc/xc_dom_bzimageloader.c --- a/tools/libxc/xc_dom_bzimageloader.c Thu Mar 10 19:15:19 2011 +0000 +++ b/tools/libxc/xc_dom_bzimageloader.c Fri Mar 11 08:31:40 2011 +0000 @@ -142,20 +142,15 @@ static int xc_try_bzip2_decode( static int _xc_try_lzma_decode( struct xc_dom_image *dom, void **blob, size_t *size, - lzma_stream *stream, lzma_ret ret, const char *what) + lzma_stream *stream, const char *what) { + lzma_ret ret; lzma_action action = LZMA_RUN; unsigned char *out_buf; unsigned char *tmp_buf; int retval = -1; int outsize; const char *msg; - - if ( ret != LZMA_OK ) - { - DOMPRINTF("%s: Failed to init decoder", what); - return -1; - } /* sigh. We don''t know up-front how much memory we are going to need * for the output buffer. Allocate the output buffer to be equal @@ -259,18 +254,28 @@ static int xc_try_xz_decode( struct xc_dom_image *dom, void **blob, size_t *size) { lzma_stream stream = LZMA_STREAM_INIT; - lzma_ret ret = lzma_stream_decoder(&stream, LZMA_BLOCK_SIZE, 0); - return _xc_try_lzma_decode(dom, blob, size, &stream, ret, "XZ"); + if ( lzma_stream_decoder(&stream, LZMA_BLOCK_SIZE, 0) != LZMA_OK ) + { + DOMPRINTF("XZ: Failed to init decoder"); + return -1; + } + + return _xc_try_lzma_decode(dom, blob, size, &stream, "XZ"); } static int xc_try_lzma_decode( struct xc_dom_image *dom, void **blob, size_t *size) { lzma_stream stream = LZMA_STREAM_INIT; - lzma_ret ret = lzma_alone_decoder(&stream, LZMA_BLOCK_SIZE); - return _xc_try_lzma_decode(dom, blob, size, &stream, ret, "LZMA"); + if ( lzma_alone_decoder(&stream, LZMA_BLOCK_SIZE) != LZMA_OK ) + { + DOMPRINTF("LZMA: Failed to init decoder"); + return -1; + } + + return _xc_try_lzma_decode(dom, blob, size, &stream, "LZMA"); } #else /* !defined(HAVE_LZMA) */ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Mar-11 18:21 UTC
Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression
Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel
decompression"):> I think you''ve taken avoidance of duplication to its illogical
extreme
> here and it is detrimental to the readability and maintainability of the
> code.
I agree and I''ve applied Ian C''s patch. Thanks, Ian.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel