Re: [PATCH v5 2/2] xen/arm: support gzip compressed kernels

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-11 at 17:20 +0100, Stefano Stabellini wrote:
> 
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  #ifdef CONFIG_ARM_64
> > >  /*
> > >   * Check if the image is a 64-bit Image.
> > > @@ -463,6 +522,22 @@ int kernel_probe(struct kernel_info *info)
> > >          printk("Loading ramdisk from boot module @ %"PRIpaddr"\n",
> > >                 info->initrd_bootmodule->start);
> > >  
> > > +    /* if it is a gzip'ed image, 32bit or 64bit, uncompress it */
> > > +    rc = kernel_decompress(info, &start, &size);
> > > +    if (rc < 0 && rc != -EINVAL)
> > 
> > IMHO kernel_decompress should return success when the decompress is a
> > nop
> > (as represented by EINVAL here) and an error only when the thing needs
> > to
> > be decompressed but cannot be.
> 
> That mean collapsing the "nothing to do" and the "decompression
> successful" cases into a single return value, which I think is not a
> good idea. We would be losing information compared to what we have now.

We aren't making any use of that information though, and nor is there any
real reason to do so in the caller.

If you care are about the distinction between "not compressed" and
"successfully decompressed" then logging "Successfully decompressed kernel"
at the end of kernel_decompress would be far more useful than returning
some errno instead of 0.

The more important bit to me though is to move the update of mod
->{size,start} and the freeing of the old memory into the decompress
function, such that it is called and returns in a completely consistent
state, rather than returning in an inconsistent state and requiring the
caller to fix it up.

Ian.