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

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

>>> 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 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel