WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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@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