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

[Ian Jackson <iwj@xxxxxxxxxxxxxx>]

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 ?

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

> The [true] in the 4th argument is there to prevent the default
> behavior of failing the configure process altogether. You'd
> see autoconf's default there only if the argument was absent.

I see.  Thanks for correcting me.  See above.

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

> > This suggests to me that a warning for missing zstd is not necessarily
> > a good idea unless it is conditional for x86.
> 
> Well, okay, I'll drop the warning then.

Thanks.

> > IDK what the zstd-defined endianness is.  I guess it must be LE for
> > your patch to work on x86.
> 
> This field is not part of the zstd output, but gets appended
> to the output by the Linux kernel build system. IOW its
> endianness gets defined by Linux; the text in the respective
> Makefile says "littleendian".

How odd.  OK.

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

Thanks,
Ian.