Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

Hi.  Thanks for this.  Firstly, RM hat: this is the tools half of zstd
decompression support which I think is a blocker for the release.  So
I am going to waive the last posting date requirement.  Therefore,

Assuming it's committed this week:

Release-Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>


Secondly, I think it would be sensible for me to review it:

> As far as configure.ac goes, I'm pretty sure there is a better (more
> "standard") way of using PKG_CHECK_MODULES().

Yes, what you have done is rather unidiomatic and seems to rely on
undocumented internals of the PKG_*. macros.  Why not do as was done
for bz2, lzma, lzo2 ?  Printing the errors to configure's terminal is
not normally done, either.

>  The construct also gets
> put next to the other decompression library checks, albeit I think they
> all ought to be x86-specific (e.g. placed in the existing case block a
> few lines down).

I don't understand why there is an x86-specific angle here.

> This follows the logic used for other decompression methods utilizing an
> external library, albeit here we can't ignore the 32-bit size field
> appended to the compressed image - its presence causes decompression to
> fail. Leverage the field instead to allocate the output buffer in one
> go, i.e. without incrementally realloc()ing.

> +    insize = *size - 4;
> +    outsize = *(uint32_t *)(*blob + insize);

Potentiallty unaligned access.  IDK if this kind of thing is thought
OK in hypervisor code but it it's not sufficiently portable for tools.

The rest of this code looks OK to me.  I spent quite a while trying to
figure out the memory management / ownership rules for the interface
to these decompression functions.  This business where they all
allocate a new buffer, and overwrite their input pointer with it (but
only on success), is pretty nasty.  I wasn't able to find where the
old buffer was freed.  But the other decompressors all seem to work
the same way.  Urgh.  In summary: nasty, but, this new code seems to
follow the existing convension.

Thanks,
Ian.