Re: [Xen-devel] [PATCH 2/3] add DomU xz kernel decompression

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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