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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2021 12:30, Ian Jackson wrote:
> 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>

Thanks.

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

Which specific part of the construct are you referring to?
I didn't think I used anything outright undocumented. Of
course I did have some trouble finding suitable docs, but in
the end I managed to locate at least something that I was
able to grok.

>  Why not do as was done for bz2, lzma, lzo2 ?

Because the pkg-config approach is more flexible - aiui
AC_CHECK_HEADER() and AC_CHECK_LIB() won't find a
dependency when sitting in some custom place, which the *.pc
files are specifically supposed to cover for.

>  Printing the errors to configure's terminal is
> not normally done, either.

With this you mean the AC_MSG_WARN()? I'm okay to drop it; I
was actually half tempted to myself already, but thought having
it would be better in line with PKG_CHECK_MODULES() when not
passed a 4th argument (where it gets quite verbose, but of
course also fails the configure process altogether).

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

On a "normal" libxenguest build decompression is available
only on x86, because of

SRCS-$(CONFIG_X86)     += xg_dom_bzimageloader.c

Hence the dependencies thereof also only ought to need
checking on x86.

I have to admit I'm uncertain about the stubdom build. I was
merely implying that if decompression is unavailable in "normal"
builds outside of x86, then _if_ non-x86 builds of stubdom exist
in the first place, decompression code there is at best dead
(the quoted restriction from Makefile applies in this case too,
and hence I can't see callers of that code, despite

ifeq ($(CONFIG_LIBXC_MINIOS),y)
SRCS-y                 += xg_dom_decompress_unsafe.c
SRCS-y                 += xg_dom_decompress_unsafe_bzip2.c
SRCS-y                 += xg_dom_decompress_unsafe_lzma.c
SRCS-y                 += xg_dom_decompress_unsafe_lzo1x.c
SRCS-y                 += xg_dom_decompress_unsafe_xz.c
SRCS-y                 += xg_dom_decompress_unsafe_zstd.c
endif

not restricting it to x86).

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

Also a possible endianness issue, yes. Since as per above this
code gets used on x86 only, I thought this would be fine at least
for now. In fact before using this simplistic approach I did
check whether xg_dom_bzimageloader.c had suitable abstraction
available, yet I couldn't spot any.

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

Yes, this isn't pretty, but looks to have served the purpose. I'd
be happy to see it improved, but I'm afraid beyond what's in this
series I won't have much time to help the overall situation.

Jan