On Fri, 2011年03月11日 at 08:20 +0000, Jan Beulich wrote:
> >>> On 11.03.11 at 08:28, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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@xxxxxxxxxx>
# 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel