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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.01.2021 15:53, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd 
> compressed kernels"):
>> On 25.01.2021 14:51, Ian Jackson wrote:
>>> I mean, the parts where you examine libzstd_PKG_ERRORS.
>>
>> The only thing I make use of is it being (non-)empty. Do you
>> really think that's a problem?
> 
> It's highly unusual.   Conceivably it might be empty even if
> pkg-config failed.
> 
>>>  AC_CHECK_LIB([lzo2], [lzo1x_decompress], [zlib="$zlib -DHAVE_LZO1X 
>>> -llzo2"])
>>> +PKG_CHECK_MODULES([libzstd], [libzstd], [zlib="$zlib -DHAVE_ZSTD 
>>> $libzstd_CFLAGS $libzstd_LIBS"])
>>
>> No, that's what I did initially, resulting in libzstd becoming
>> a strict requirement (i.e. configure failing if it's absent),
>> as reported by Michael Young.
> 
> Well how about passing "true" for the fourth argument then ?

That I did try intermediately, but didn't ever post. It'll
screw up when libzstd_CFLAGS and libzstd_LIBS were provided
to override pkg-config. When you look at the expanded code,
this will end up with pkg_failed set to "untried" and still
take the error path. I.e. we wouldn't get the overridden
settings appended to $zlib.

>>> I mean the inclusion of $libzstd_PKG_ERRORS in the output.
>>
>> I see no point in the warning without including this. In fact
>> I added the AC_MSG_WARN() just so that the contents of this
>> variable (and hence an indication to the user of what to do)
>> was easily accessible.
> 
> This is not usual autoconf practice.  The usual approach is to
> consider that missing features are just to be dealt with with a
> minimum of fuss.

Which is why I made the description say what it says. Just
that - as per above - I don't see viable alternatives (yet).

>>> If you want a warning I think it should be a call to AC_MSG_WARN in
>>> ACTION-IF-NOT-FOUND.
>>
>> I didn't to avoid the nesting of things yielding even harder
>> to read code.
> 
> In your code it's nested too, just in an if rather than the in the
> macro argument - but with a separate condition.  Please do it the
> "usual autoconf way".

Pieces of shell code look to be permitted - a few lines down
from the addition to configure.ac there is a shell case
statement. Or are you telling me that's an abuse I shouldn't
follow? But then I still don't see how to sensibly replace
the construct, given the issue described further up.

>>> How unfortunate.  I have also hunted for some existing code and also
>>> didn't find anything suitably general.
>>>
>>> I did find this, open-coded in xg_dom_core.c:xc_dom_check_gzip:
>>>
>>>     unziplen = (size_t)gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | 
>>> gzlen[0];
>>
>> Okay, I'll copy that then.
> 
> Could you make a macro or inline function in xg_private.h[1] rather
> than open-coding a copy, please ?
> 
> [1] Or, if you prefer, a header with wider scope.

I can, but it feels wrong, in particular if I gave it a
generic looking name (get_unaligned_le32() or some such, if
I was to follow the kernel-originating(?) approach used in
the mini-os wrapping of the hypervisor decompressor code),
and something like get_linuxes_idea_of_uncompressed_size()
is also, well, not really nice. Especially if put in a
general header like xg_private.h (i.e. in this latter case
I'd rather see the helper have more narrow scope, but of
course introducing a new header just for this seems overkill
as well). Any more concrete suggestion would be appreciated
here.

Jan