[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.