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