[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |