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.

As far as configure.ac goes, I'm pretty sure there is a better (more
"standard") way of using PKG_CHECK_MODULES(). 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).

Note that, where possible, instead of #ifdef-ing xen/*.h inclusions,
they get removed.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wl@xen.org>
---
Note for committer: As an alternative to patching tools/configure here,
autoconf may want re-running.
---
v2.5: Don't make libzstd a hard dependency. Adjust ./README.
v2: New.

--- a/README
+++ b/README
@@ -84,6 +84,8 @@ disabled at compile time:
     * 16-bit x86 assembler, loader and compiler for qemu-traditional / rombios
       (dev86 rpm or bin86 & bcc debs)
     * Development install of liblzma for rombios
+    * Development install of libbz2, liblzma, liblzo2, and libzstd for DomU
+      kernel decompression.
 
 Second, you need to acquire a suitable kernel for use in domain 0. If
 possible you should use a kernel provided by your OS distributor. If
--- a/tools/configure
+++ b/tools/configure
@@ -643,6 +643,8 @@ PTHREAD_CFLAGS
 EXTFS_LIBS
 system_aio
 zlib
+libzstd_LIBS
+libzstd_CFLAGS
 FETCHER
 FTP
 FALSE
@@ -857,6 +859,8 @@ glib_CFLAGS
 glib_LIBS
 pixman_CFLAGS
 pixman_LIBS
+libzstd_CFLAGS
+libzstd_LIBS
 LIBNL3_CFLAGS
 LIBNL3_LIBS
 SYSTEMD_CFLAGS
@@ -1605,6 +1609,10 @@ Some influential environment variables:
   pixman_CFLAGS
               C compiler flags for pixman, overriding pkg-config
   pixman_LIBS linker flags for pixman, overriding pkg-config
+  libzstd_CFLAGS
+              C compiler flags for libzstd, overriding pkg-config
+  libzstd_LIBS
+              linker flags for libzstd, overriding pkg-config
   LIBNL3_CFLAGS
               C compiler flags for LIBNL3, overriding pkg-config
   LIBNL3_LIBS linker flags for LIBNL3, overriding pkg-config
@@ -8744,6 +8752,84 @@ fi
 
 
 
+pkg_failed=no
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for libzstd" >&5
+$as_echo_n "checking for libzstd... " >&6; }
+
+if test -n "$libzstd_CFLAGS"; then
+    pkg_cv_libzstd_CFLAGS="$libzstd_CFLAGS"
+ elif test -n "$PKG_CONFIG"; then
+    if test -n "$PKG_CONFIG" && \
+    { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libzstd\""; } >&5
+  ($PKG_CONFIG --exists --print-errors "libzstd") 2>&5
+  ac_status=$?
+  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
+  test $ac_status = 0; }; then
+  pkg_cv_libzstd_CFLAGS=`$PKG_CONFIG --cflags "libzstd" 2>/dev/null`
+		      test "x$?" != "x0" && pkg_failed=yes
+else
+  pkg_failed=yes
+fi
+ else
+    pkg_failed=untried
+fi
+if test -n "$libzstd_LIBS"; then
+    pkg_cv_libzstd_LIBS="$libzstd_LIBS"
+ elif test -n "$PKG_CONFIG"; then
+    if test -n "$PKG_CONFIG" && \
+    { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists --print-errors \"libzstd\""; } >&5
+  ($PKG_CONFIG --exists --print-errors "libzstd") 2>&5
+  ac_status=$?
+  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
+  test $ac_status = 0; }; then
+  pkg_cv_libzstd_LIBS=`$PKG_CONFIG --libs "libzstd" 2>/dev/null`
+		      test "x$?" != "x0" && pkg_failed=yes
+else
+  pkg_failed=yes
+fi
+ else
+    pkg_failed=untried
+fi
+
+
+
+if test $pkg_failed = yes; then
+   	{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+
+if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then
+        _pkg_short_errors_supported=yes
+else
+        _pkg_short_errors_supported=no
+fi
+        if test $_pkg_short_errors_supported = yes; then
+	        libzstd_PKG_ERRORS=`$PKG_CONFIG --short-errors --print-errors --cflags --libs "libzstd" 2>&1`
+        else
+	        libzstd_PKG_ERRORS=`$PKG_CONFIG --print-errors --cflags --libs "libzstd" 2>&1`
+        fi
+	# Put the nasty error message in config.log where it belongs
+	echo "$libzstd_PKG_ERRORS" >&5
+
+	true
+elif test $pkg_failed = untried; then
+     	{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+	true
+else
+	libzstd_CFLAGS=$pkg_cv_libzstd_CFLAGS
+	libzstd_LIBS=$pkg_cv_libzstd_LIBS
+        { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
+$as_echo "yes" >&6; }
+	true
+fi
+if test -z "$libzstd_PKG_ERRORS"
+then
+    zlib="$zlib -DHAVE_ZSTD $libzstd_CFLAGS $libzstd_LIBS"
+else
+    { $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: \"disabling zstd decompression: $libzstd_PKG_ERRORS\"" >&5
+$as_echo "$as_me: WARNING: \"disabling zstd decompression: $libzstd_PKG_ERRORS\"" >&2;}
+fi
+
 
 
 ac_fn_c_check_header_mongrel "$LINENO" "ext2fs/ext2fs.h" "ac_cv_header_ext2fs_ext2fs_h" "$ac_includes_default"
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -414,6 +414,13 @@ AC_CHECK_LIB([lzma], [lzma_stream_decode
 AC_CHECK_HEADER([lzo/lzo1x.h], [
 AC_CHECK_LIB([lzo2], [lzo1x_decompress], [zlib="$zlib -DHAVE_LZO1X -llzo2"])
 ])
+PKG_CHECK_MODULES([libzstd], [libzstd], [true], [true])
+if test -z "$libzstd_PKG_ERRORS"
+then
+    zlib="$zlib -DHAVE_ZSTD $libzstd_CFLAGS $libzstd_LIBS"
+else
+    AC_MSG_WARN("disabling zstd decompression: $libzstd_PKG_ERRORS")
+fi
 AC_SUBST(zlib)
 AC_SUBST(system_aio)
 AX_CHECK_EXTFS
--- a/tools/libs/guest/Makefile
+++ b/tools/libs/guest/Makefile
@@ -64,6 +64,7 @@ SRCS-y                 += xg_dom_decompr
 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
 
 -include $(XEN_TARGET_ARCH)/Makefile
--- a/tools/libs/guest/xg_dom_bzimageloader.c
+++ b/tools/libs/guest/xg_dom_bzimageloader.c
@@ -589,6 +589,85 @@ static int xc_try_lzo1x_decode(
 
 #endif
 
+#if defined(HAVE_ZSTD)
+
+#include <zstd.h>
+
+static int xc_try_zstd_decode(
+    struct xc_dom_image *dom, void **blob, size_t *size)
+{
+    size_t outsize, insize, actual;
+    unsigned char *outbuf;
+
+    /* Magic, descriptor byte, and trailing size field. */
+    if ( *size <= 9 )
+    {
+        DOMPRINTF("ZSTD: insufficient input data");
+        return -1;
+    }
+
+    insize = *size - 4;
+    outsize = *(uint32_t *)(*blob + insize);
+
+    if ( xc_dom_kernel_check_size(dom, outsize) )
+    {
+        DOMPRINTF("ZSTD: output too large");
+        return -1;
+    }
+
+    outbuf = malloc(outsize);
+    if ( !outbuf )
+    {
+        DOMPRINTF("ZSTD: failed to alloc memory");
+        return -1;
+    }
+
+    actual = ZSTD_decompress(outbuf, outsize, *blob, insize);
+
+    if ( ZSTD_isError(actual) )
+    {
+        DOMPRINTF("ZSTD: error: %s", ZSTD_getErrorName(actual));
+        free(outbuf);
+        return -1;
+    }
+
+    if ( actual != outsize )
+    {
+        DOMPRINTF("ZSTD: got 0x%zx bytes instead of 0x%zx",
+                  actual, outsize);
+        free(outbuf);
+        return -1;
+    }
+
+    if ( xc_dom_register_external(dom, outbuf, outsize) )
+    {
+        DOMPRINTF("ZSTD: error registering stream output");
+        free(outbuf);
+        return -1;
+    }
+
+    DOMPRINTF("%s: ZSTD decompress OK, 0x%zx -> 0x%zx",
+              __FUNCTION__, insize, outsize);
+
+    *blob = outbuf;
+    *size = outsize;
+
+    return 0;
+}
+
+#else /* !defined(HAVE_ZSTD) */
+
+static int xc_try_zstd_decode(
+    struct xc_dom_image *dom, void **blob, size_t *size)
+{
+    xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
+                 "%s: ZSTD decompress support unavailable\n",
+                 __FUNCTION__);
+    return -1;
+}
+
+#endif
+
 #else /* __MINIOS__ */
 
 int xc_try_bzip2_decode(struct xc_dom_image *dom, void **blob, size_t *size);
@@ -735,6 +814,17 @@ static int xc_dom_probe_bzimage_kernel(s
                          __FUNCTION__);
             return -EINVAL;
         }
+    }
+    else if ( check_magic(dom, "\x28\xb5\x2f\xfd", 4) )
+    {
+        ret = xc_try_zstd_decode(dom, &dom->kernel_blob, &dom->kernel_size);
+        if ( ret < 0 )
+        {
+            xc_dom_panic(dom->xch, XC_INVALID_KERNEL,
+                         "%s unable to ZSTD decompress kernel",
+                         __FUNCTION__);
+            return -EINVAL;
+        }
     }
     else if ( check_magic(dom, "\135\000", 2) )
     {
--- /dev/null
+++ b/tools/libs/guest/xg_dom_decompress_unsafe_zstd.c
@@ -0,0 +1,45 @@
+#include <stdio.h>
+#include <endian.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <inttypes.h>
+
+#include "xg_private.h"
+#include "xg_dom_decompress_unsafe.h"
+
+typedef uint8_t u8;
+
+typedef uint16_t __u16;
+typedef uint32_t __u32;
+typedef uint64_t __u64;
+
+typedef uint16_t __le16;
+typedef uint32_t __le32;
+typedef uint64_t __le64;
+
+typedef uint16_t __be16;
+typedef uint32_t __be32;
+typedef uint64_t __be64;
+
+#define __attribute_const__
+#define __force
+#define always_inline
+#define noinline
+
+#undef ERROR
+
+#define __BYTEORDER_HAS_U64__
+#define __TYPES_H__ /* xen/types.h guard */
+#include "../../xen/include/xen/byteorder/little_endian.h"
+#define __ASM_UNALIGNED_H__ /* asm/unaligned.h guard */
+#include "../../xen/include/xen/unaligned.h"
+#include "../../xen/include/xen/xxhash.h"
+#include "../../xen/lib/xxhash64.c"
+#include "../../xen/common/unzstd.c"
+
+int xc_try_zstd_decode(
+    struct xc_dom_image *dom, void **blob, size_t *size)
+{
+    return xc_dom_decompress_unsafe(unzstd, dom, blob, size);
+}
--- a/tools/libs/guest/xg_dom_decompress_unsafe.h
+++ b/tools/libs/guest/xg_dom_decompress_unsafe.h
@@ -16,3 +16,5 @@ int xc_try_lzo1x_decode(struct xc_dom_im
     __attribute__((visibility("internal")));
 int xc_try_xz_decode(struct xc_dom_image *dom, void **blob, size_t *size)
     __attribute__((visibility("internal")));
+int xc_try_zstd_decode(struct xc_dom_image *dom, void **blob, size_t *size)
+    __attribute__((visibility("internal")));
--- a/xen/common/zstd/decompress.c
+++ b/xen/common/zstd/decompress.c
@@ -33,7 +33,6 @@
 #include "huf.h"
 #include "mem.h" /* low level memory routines */
 #include "zstd_internal.h"
-#include <xen/string.h> /* memcpy, memmove, memset */
 
 #define ZSTD_PREFETCH(ptr) __builtin_prefetch(ptr, 0, 0)
 
@@ -99,9 +98,12 @@ struct ZSTD_DCtx_s {
 	BYTE headerBuffer[ZSTD_FRAMEHEADERSIZE_MAX];
 }; /* typedef'd to ZSTD_DCtx within "zstd.h" */
 
-size_t INIT ZSTD_DCtxWorkspaceBound(void) { return ZSTD_ALIGN(sizeof(ZSTD_stack)) + ZSTD_ALIGN(sizeof(ZSTD_DCtx)); }
+STATIC size_t INIT ZSTD_DCtxWorkspaceBound(void)
+{
+	return ZSTD_ALIGN(sizeof(ZSTD_stack)) + ZSTD_ALIGN(sizeof(ZSTD_DCtx));
+}
 
-size_t INIT ZSTD_decompressBegin(ZSTD_DCtx *dctx)
+STATIC size_t INIT ZSTD_decompressBegin(ZSTD_DCtx *dctx)
 {
 	dctx->expected = ZSTD_frameHeaderSize_prefix;
 	dctx->stage = ZSTDds_getFrameHeaderSize;
@@ -121,7 +123,7 @@ size_t INIT ZSTD_decompressBegin(ZSTD_DC
 	return 0;
 }
 
-ZSTD_DCtx *INIT ZSTD_createDCtx_advanced(ZSTD_customMem customMem)
+STATIC ZSTD_DCtx *INIT ZSTD_createDCtx_advanced(ZSTD_customMem customMem)
 {
 	ZSTD_DCtx *dctx;
 
@@ -136,7 +138,7 @@ ZSTD_DCtx *INIT ZSTD_createDCtx_advanced
 	return dctx;
 }
 
-ZSTD_DCtx *INIT ZSTD_initDCtx(void *workspace, size_t workspaceSize)
+STATIC ZSTD_DCtx *INIT ZSTD_initDCtx(void *workspace, size_t workspaceSize)
 {
 	ZSTD_customMem const stackMem = ZSTD_initStack(workspace, workspaceSize);
 	return ZSTD_createDCtx_advanced(stackMem);
@@ -150,11 +152,13 @@ size_t INIT ZSTD_freeDCtx(ZSTD_DCtx *dct
 	return 0; /* reserved as a potential error code in the future */
 }
 
+#ifdef BUILD_DEAD_CODE
 void INIT ZSTD_copyDCtx(ZSTD_DCtx *dstDCtx, const ZSTD_DCtx *srcDCtx)
 {
 	size_t const workSpaceSize = (ZSTD_BLOCKSIZE_ABSOLUTEMAX + WILDCOPY_OVERLENGTH) + ZSTD_frameHeaderSize_max;
 	memcpy(dstDCtx, srcDCtx, sizeof(ZSTD_DCtx) - workSpaceSize); /* no need to copy workspace */
 }
+#endif
 
 STATIC size_t ZSTD_findFrameCompressedSize(const void *src, size_t srcSize);
 STATIC size_t ZSTD_decompressBegin_usingDict(ZSTD_DCtx *dctx, const void *dict,
@@ -166,6 +170,7 @@ static void ZSTD_refDDict(ZSTD_DCtx *dst
 *   Decompression section
 ***************************************************************/
 
+#ifdef BUILD_DEAD_CODE
 /*! ZSTD_isFrame() :
  *  Tells if the content of `buffer` starts with a valid Frame Identifier.
  *  Note : Frame Identifier is 4 bytes. If `size < 4`, @return will always be 0.
@@ -184,6 +189,7 @@ unsigned INIT ZSTD_isFrame(const void *b
 	}
 	return 0;
 }
+#endif
 
 /** ZSTD_frameHeaderSize() :
 *   srcSize must be >= ZSTD_frameHeaderSize_prefix.
@@ -206,7 +212,7 @@ static size_t INIT ZSTD_frameHeaderSize(
 *   @return : 0, `fparamsPtr` is correctly filled,
 *            >0, `srcSize` is too small, result is expected `srcSize`,
 *             or an error code, which can be tested using ZSTD_isError() */
-size_t INIT ZSTD_getFrameParams(ZSTD_frameParams *fparamsPtr, const void *src, size_t srcSize)
+STATIC size_t INIT ZSTD_getFrameParams(ZSTD_frameParams *fparamsPtr, const void *src, size_t srcSize)
 {
 	const BYTE *ip = (const BYTE *)src;
 
@@ -291,6 +297,7 @@ size_t INIT ZSTD_getFrameParams(ZSTD_fra
 	return 0;
 }
 
+#ifdef BUILD_DEAD_CODE
 /** ZSTD_getFrameContentSize() :
 *   compatible with legacy mode
 *   @return : decompressed size of the single frame pointed to be `src` if known, otherwise
@@ -367,6 +374,7 @@ unsigned long long INIT ZSTD_findDecompr
 		return totalDstSize;
 	}
 }
+#endif /* BUILD_DEAD_CODE */
 
 /** ZSTD_decodeFrameHeader() :
 *   `headerSize` must be the size provided by ZSTD_frameHeaderSize().
@@ -393,7 +401,7 @@ typedef struct {
 
 /*! ZSTD_getcBlockSize() :
 *   Provides the size of compressed block from block header `src` */
-size_t INIT ZSTD_getcBlockSize(const void *src, size_t srcSize, blockProperties_t *bpPtr)
+STATIC size_t INIT ZSTD_getcBlockSize(const void *src, size_t srcSize, blockProperties_t *bpPtr)
 {
 	if (srcSize < ZSTD_blockHeaderSize)
 		return ERROR(srcSize_wrong);
@@ -431,7 +439,7 @@ static size_t INIT ZSTD_setRleBlock(void
 
 /*! ZSTD_decodeLiteralsBlock() :
 	@return : nb of bytes read from src (< srcSize ) */
-size_t INIT ZSTD_decodeLiteralsBlock(ZSTD_DCtx *dctx, const void *src, size_t srcSize) /* note : srcSize < BLOCKSIZE */
+STATIC size_t INIT ZSTD_decodeLiteralsBlock(ZSTD_DCtx *dctx, const void *src, size_t srcSize) /* note : srcSize < BLOCKSIZE */
 {
 	if (srcSize < MIN_CBLOCK_SIZE)
 		return ERROR(corruption_detected);
@@ -795,7 +803,7 @@ static size_t INIT ZSTD_buildSeqTable(FS
 	}
 }
 
-size_t INIT ZSTD_decodeSeqHeaders(ZSTD_DCtx *dctx, int *nbSeqPtr, const void *src, size_t srcSize)
+STATIC size_t INIT ZSTD_decodeSeqHeaders(ZSTD_DCtx *dctx, int *nbSeqPtr, const void *src, size_t srcSize)
 {
 	const BYTE *const istart = (const BYTE *const)src;
 	const BYTE *const iend = istart + srcSize;
@@ -1481,6 +1489,7 @@ static void INIT ZSTD_checkContinuity(ZS
 	}
 }
 
+#ifdef BUILD_DEAD_CODE
 size_t INIT ZSTD_decompressBlock(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize)
 {
 	size_t dSize;
@@ -1498,8 +1507,9 @@ size_t INIT ZSTD_insertBlock(ZSTD_DCtx *
 	dctx->previousDstEnd = (const char *)blockStart + blockSize;
 	return blockSize;
 }
+#endif /* BUILD_DEAD_CODE */
 
-size_t INIT ZSTD_generateNxBytes(void *dst, size_t dstCapacity, BYTE byte, size_t length)
+STATIC size_t INIT ZSTD_generateNxBytes(void *dst, size_t dstCapacity, BYTE byte, size_t length)
 {
 	if (length > dstCapacity)
 		return ERROR(dstSize_tooSmall);
@@ -1512,7 +1522,7 @@ size_t INIT ZSTD_generateNxBytes(void *d
  *  `src` must point to the start of a ZSTD frame, ZSTD legacy frame, or skippable frame
  *  `srcSize` must be at least as large as the frame contained
  *  @return : the compressed size of the frame starting at `src` */
-size_t INIT ZSTD_findFrameCompressedSize(const void *src, size_t srcSize)
+STATIC size_t INIT ZSTD_findFrameCompressedSize(const void *src, size_t srcSize)
 {
 	if (srcSize >= ZSTD_skippableHeaderSize && (ZSTD_readLE32(src) & 0xFFFFFFF0U) == ZSTD_MAGIC_SKIPPABLE_START) {
 		return ZSTD_skippableHeaderSize + ZSTD_readLE32((const BYTE *)src + 4);
@@ -1709,12 +1719,12 @@ static size_t INIT ZSTD_decompressMultiF
 	return (BYTE *)dst - (BYTE *)dststart;
 }
 
-size_t INIT ZSTD_decompress_usingDict(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize, const void *dict, size_t dictSize)
+STATIC size_t INIT ZSTD_decompress_usingDict(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize, const void *dict, size_t dictSize)
 {
 	return ZSTD_decompressMultiFrame(dctx, dst, dstCapacity, src, srcSize, dict, dictSize, NULL);
 }
 
-size_t INIT ZSTD_decompressDCtx(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize)
+STATIC size_t INIT ZSTD_decompressDCtx(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize)
 {
 	return ZSTD_decompress_usingDict(dctx, dst, dstCapacity, src, srcSize, NULL, 0);
 }
@@ -1723,9 +1733,12 @@ size_t INIT ZSTD_decompressDCtx(ZSTD_DCt
 *   Advanced Streaming Decompression API
 *   Bufferless and synchronous
 ****************************************/
-size_t INIT ZSTD_nextSrcSizeToDecompress(ZSTD_DCtx *dctx) { return dctx->expected; }
+STATIC size_t INIT ZSTD_nextSrcSizeToDecompress(ZSTD_DCtx *dctx)
+{
+	return dctx->expected;
+}
 
-ZSTD_nextInputType_e INIT ZSTD_nextInputType(ZSTD_DCtx *dctx)
+STATIC ZSTD_nextInputType_e INIT ZSTD_nextInputType(ZSTD_DCtx *dctx)
 {
 	switch (dctx->stage) {
 	default: /* should not happen */
@@ -1745,7 +1758,7 @@ int INIT ZSTD_isSkipFrame(ZSTD_DCtx *dct
 /** ZSTD_decompressContinue() :
 *   @return : nb of bytes generated into `dst` (necessarily <= `dstCapacity)
 *             or an error code, which can be tested using ZSTD_isError() */
-size_t INIT ZSTD_decompressContinue(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize)
+STATIC size_t INIT ZSTD_decompressContinue(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize)
 {
 	/* Sanity check */
 	if (srcSize != dctx->expected)
@@ -1971,7 +1984,7 @@ static size_t INIT ZSTD_decompress_inser
 	return ZSTD_refDictContent(dctx, dict, dictSize);
 }
 
-size_t INIT ZSTD_decompressBegin_usingDict(ZSTD_DCtx *dctx, const void *dict, size_t dictSize)
+STATIC size_t INIT ZSTD_decompressBegin_usingDict(ZSTD_DCtx *dctx, const void *dict, size_t dictSize)
 {
 	CHECK_F(ZSTD_decompressBegin(dctx));
 	if (dict && dictSize)
@@ -1991,7 +2004,9 @@ struct ZSTD_DDict_s {
 	ZSTD_customMem cMem;
 }; /* typedef'd to ZSTD_DDict within "zstd.h" */
 
+#ifdef BUILD_DEAD_CODE
 size_t INIT ZSTD_DDictWorkspaceBound(void) { return ZSTD_ALIGN(sizeof(ZSTD_stack)) + ZSTD_ALIGN(sizeof(ZSTD_DDict)); }
+#endif
 
 static const void *INIT ZSTD_DDictDictContent(const ZSTD_DDict *ddict) { return ddict->dictContent; }
 
@@ -2023,6 +2038,7 @@ static void INIT ZSTD_refDDict(ZSTD_DCtx
 	}
 }
 
+#ifdef BUILD_DEAD_CODE
 static size_t INIT ZSTD_loadEntropy_inDDict(ZSTD_DDict *ddict)
 {
 	ddict->dictID = 0;
@@ -2090,6 +2106,7 @@ ZSTD_DDict *INIT ZSTD_initDDict(const vo
 	ZSTD_customMem const stackMem = ZSTD_initStack(workspace, workspaceSize);
 	return ZSTD_createDDict_advanced(dict, dictSize, 1, stackMem);
 }
+#endif /* BUILD_DEAD_CODE */
 
 size_t INIT ZSTD_freeDDict(ZSTD_DDict *ddict)
 {
@@ -2103,6 +2120,7 @@ size_t INIT ZSTD_freeDDict(ZSTD_DDict *d
 	}
 }
 
+#ifdef BUILD_DEAD_CODE
 /*! ZSTD_getDictID_fromDict() :
  *  Provides the dictID stored within dictionary.
  *  if @return == 0, the dictionary is not conformant with Zstandard specification.
@@ -2145,11 +2163,12 @@ unsigned INIT ZSTD_getDictID_fromFrame(c
 		return 0;
 	return zfp.dictID;
 }
+#endif /* BUILD_DEAD_CODE */
 
 /*! ZSTD_decompress_usingDDict() :
 *   Decompression using a pre-digested Dictionary
 *   Use dictionary without significant overhead. */
-size_t INIT ZSTD_decompress_usingDDict(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize, const ZSTD_DDict *ddict)
+STATIC size_t INIT ZSTD_decompress_usingDDict(ZSTD_DCtx *dctx, void *dst, size_t dstCapacity, const void *src, size_t srcSize, const ZSTD_DDict *ddict)
 {
 	/* pass content and size in case legacy frames are encountered */
 	return ZSTD_decompressMultiFrame(dctx, dst, dstCapacity, src, srcSize, NULL, 0, ddict);
@@ -2186,7 +2205,7 @@ struct ZSTD_DStream_s {
 	U32 hostageByte;
 }; /* typedef'd to ZSTD_DStream within "zstd.h" */
 
-size_t INIT ZSTD_DStreamWorkspaceBound(size_t maxWindowSize)
+STATIC size_t INIT ZSTD_DStreamWorkspaceBound(size_t maxWindowSize)
 {
 	size_t const blockSize = MIN(maxWindowSize, ZSTD_BLOCKSIZE_ABSOLUTEMAX);
 	size_t const inBuffSize = blockSize;
@@ -2216,7 +2235,7 @@ static ZSTD_DStream *INIT ZSTD_createDSt
 	return zds;
 }
 
-ZSTD_DStream *INIT ZSTD_initDStream(size_t maxWindowSize, void *workspace, size_t workspaceSize)
+STATIC ZSTD_DStream *INIT ZSTD_initDStream(size_t maxWindowSize, void *workspace, size_t workspaceSize)
 {
 	ZSTD_customMem const stackMem = ZSTD_initStack(workspace, workspaceSize);
 	ZSTD_DStream *zds = ZSTD_createDStream_advanced(stackMem);
@@ -2249,6 +2268,7 @@ ZSTD_DStream *INIT ZSTD_initDStream(size
 	return zds;
 }
 
+#ifdef BUILD_DEAD_CODE
 ZSTD_DStream *INIT ZSTD_initDStream_usingDDict(size_t maxWindowSize, const ZSTD_DDict *ddict, void *workspace, size_t workspaceSize)
 {
 	ZSTD_DStream *zds = ZSTD_initDStream(maxWindowSize, workspace, workspaceSize);
@@ -2257,6 +2277,7 @@ ZSTD_DStream *INIT ZSTD_initDStream_usin
 	}
 	return zds;
 }
+#endif
 
 size_t INIT ZSTD_freeDStream(ZSTD_DStream *zds)
 {
@@ -2279,10 +2300,12 @@ size_t INIT ZSTD_freeDStream(ZSTD_DStrea
 
 /* *** Initialization *** */
 
+#ifdef BUILD_DEAD_CODE
 size_t INIT ZSTD_DStreamInSize(void) { return ZSTD_BLOCKSIZE_ABSOLUTEMAX + ZSTD_blockHeaderSize; }
 size_t INIT ZSTD_DStreamOutSize(void) { return ZSTD_BLOCKSIZE_ABSOLUTEMAX; }
+#endif
 
-size_t INIT ZSTD_resetDStream(ZSTD_DStream *zds)
+STATIC size_t INIT ZSTD_resetDStream(ZSTD_DStream *zds)
 {
 	zds->stage = zdss_loadHeader;
 	zds->lhSize = zds->inPos = zds->outStart = zds->outEnd = 0;
@@ -2300,7 +2323,7 @@ ZSTD_STATIC size_t INIT ZSTD_limitCopy(v
 	return length;
 }
 
-size_t INIT ZSTD_decompressStream(ZSTD_DStream *zds, ZSTD_outBuffer *output, ZSTD_inBuffer *input)
+STATIC size_t INIT ZSTD_decompressStream(ZSTD_DStream *zds, ZSTD_outBuffer *output, ZSTD_inBuffer *input)
 {
 	const char *const istart = (const char *)(input->src) + input->pos;
 	const char *const iend = (const char *)(input->src) + input->size;
--- a/xen/common/zstd/error_private.h
+++ b/xen/common/zstd/error_private.h
@@ -19,11 +19,6 @@
 #ifndef ERROR_H_MODULE
 #define ERROR_H_MODULE
 
-/* ****************************************
-*  Dependencies
-******************************************/
-#include <xen/types.h> /* size_t */
-
 /**
  * enum ZSTD_ErrorCode - zstd error codes
  *
--- a/xen/common/zstd/fse.h
+++ b/xen/common/zstd/fse.h
@@ -41,11 +41,6 @@
 #define FSE_H
 
 /*-*****************************************
-*  Dependencies
-******************************************/
-#include <xen/types.h> /* size_t, ptrdiff_t */
-
-/*-*****************************************
 *  FSE_PUBLIC_API : control library symbols visibility
 ******************************************/
 #define FSE_PUBLIC_API
--- a/xen/common/zstd/fse_decompress.c
+++ b/xen/common/zstd/fse_decompress.c
@@ -48,8 +48,6 @@
 #include "bitstream.h"
 #include "fse.h"
 #include "zstd_internal.h"
-#include <xen/compiler.h>
-#include <xen/string.h> /* memcpy, memset */
 
 /* **************************************************************
 *  Error Management
--- a/xen/common/zstd/huf.h
+++ b/xen/common/zstd/huf.h
@@ -40,9 +40,6 @@
 #ifndef HUF_H_298734234
 #define HUF_H_298734234
 
-/* *** Dependencies *** */
-#include <xen/types.h> /* size_t */
-
 /* ***   Tool functions *** */
 #define HUF_BLOCKSIZE_MAX (128 * 1024) /**< maximum input size for a single block compressed with HUF_compress */
 size_t HUF_compressBound(size_t size); /**< maximum compressed size (worst case) */
--- a/xen/common/zstd/huf_decompress.c
+++ b/xen/common/zstd/huf_decompress.c
@@ -48,8 +48,6 @@
 #include "bitstream.h" /* BIT_* */
 #include "fse.h"       /* header compression */
 #include "huf.h"
-#include <xen/compiler.h>
-#include <xen/string.h> /* memcpy, memset */
 
 /* **************************************************************
 *  Error Management
--- a/xen/common/zstd/mem.h
+++ b/xen/common/zstd/mem.h
@@ -20,9 +20,11 @@
 /*-****************************************
 *  Dependencies
 ******************************************/
+#ifdef __XEN__
 #include <xen/string.h> /* memcpy */
 #include <xen/types.h>  /* size_t, ptrdiff_t */
 #include <asm/unaligned.h>
+#endif
 
 /*-****************************************
 *  Compiler specifics
--- a/xen/common/zstd/zstd_internal.h
+++ b/xen/common/zstd/zstd_internal.h
@@ -28,8 +28,10 @@
 ***************************************/
 #include "error_private.h"
 #include "mem.h"
+#ifdef __XEN__
 #include <xen/compiler.h>
 #include <xen/xxhash.h>
+#endif
 
 #define ALIGN(x, a) ((x + (a) - 1) & ~((a) - 1))
 #define PTR_ALIGN(p, a) ((typeof(p))ALIGN((unsigned long)(p), (a)))
@@ -95,8 +97,10 @@ typedef struct ZSTD_DStream_s ZSTD_DStre
 /*-*************************************
 *  shared macros
 ***************************************/
+#ifndef MIN
 #define MIN(a, b) ((a) < (b) ? (a) : (b))
 #define MAX(a, b) ((a) > (b) ? (a) : (b))
+#endif
 #define CHECK_F(f)                       \
 	{                                \
 		size_t const errcod = f; \
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -10,8 +10,10 @@
 #ifndef __XEN_UNALIGNED_H__
 #define __XEN_UNALIGNED_H__
 
+#ifdef __XEN__
 #include <xen/types.h>
 #include <asm/byteorder.h>
+#endif
 
 #define get_unaligned(p) (*(p))
 #define put_unaligned(val, p) (*(p) = (val))
--- a/xen/lib/xxhash64.c
+++ b/xen/lib/xxhash64.c
@@ -38,11 +38,13 @@
  * - xxHash source repository: https://github.com/Cyan4973/xxHash
  */
 
+#ifdef __XEN__
 #include <xen/compiler.h>
 #include <xen/errno.h>
 #include <xen/string.h>
 #include <xen/xxhash.h>
 #include <asm/unaligned.h>
+#endif
 
 /*-*************************************
  * Macros

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@eu.citrix.com>


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.  Why not do as was done
for bz2, lzma, lzo2 ?  Printing the errors to configure's terminal is
not normally done, either.

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

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

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.

Thanks,
Ian.

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@eu.citrix.com>

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

Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
> On 25.01.2021 12:30, Ian Jackson wrote:
> >> 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.

I mean, the parts where you examine libzstd_PKG_ERRORS.

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

Yes, sorry, I didn't mean to suggest that the use of PKG_CHECK_MODULES
rather than AC_CHECK_LIB was wrong.  But I think you can just pass
similar if-found and if-not-found fragments.  Maybe something like:

 AC_CHECK_LIB([lzo2], [lzo1x_decompress], [zlib="$zlib -DHAVE_LZO1X -llzo2"])
+PKG_CHECK_MODULES([libzstd], [libzstd], [zlib="$zlib -DHAVE_ZSTD $libzstd_CFLAGS $libzstd_LIBS"])

> >  Printing the errors to configure's terminal is
> > not normally done, either.
> 
> With this you mean the AC_MSG_WARN()?

I don't mind there being a call to AC_MSG_WARN.  I don't think I have
a strong opinion about whether lack of zstd ought to produce a
warning.  If there ought to be a warning, then it ought to be made
with AC_MSG_WARN, indeed.

I mean the inclusion of $libzstd_PKG_ERRORS in the output.

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

Does it ?  Admittedly the documentation I found in pkg.m4 for these
PKG_* macros doesn't say what the default is for ACTION-IF-NOT-FOUND
but it would surely parallel all the autoconf-provided macros where
the default is a no-op.  I read the autoconf output in your patch
(where admitteedly you pass [true]) and that seems to support my
supposition.

If you want a warning I think it should be a call to AC_MSG_WARN in
ACTION-IF-NOT-FOUND.

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

Oh!

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

I see.  Hmm.  TBH this seems anomalous.  I would prefer to keep the
configure test and expect that eventually some non-x86 folsk will
decide to turn this on there too.

This suggests to me that a warning for missing zstd is not necessarily
a good idea unless it is conditional for 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).

I think there is no mini-os and no stubdom build on ARM.  I don't
think this is necessarily for any particularly principled reason
except that minios in particular is not so easy to port.

So that would explain why the build isn't broken despite this
inconsistency.

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

The endianness issue at least just means "this code doesn't work and
will always reject images".  The alignment issue might mean "feeding
a corrupted image file will crash your management daemon".

IDK what the zstd-defined endianness is.  I guess it must be LE for
your patch to work on x86.

> Since as per above this
> code gets used on x86 only, I thought this would be fine at least
> for now.

I think that's too much of a boobytrap to leave in the code.

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

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];

Maybe this could be moved to a macro or inline function in
xg_private.h ?

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

Quite so.  I'm certainly not suggesting that untangling that is a
blocker for this patch.

Thanks,
Ian.

On 25.01.2021 14:51, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
>> On 25.01.2021 12:30, Ian Jackson wrote:
>>>> 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.
> 
> 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?

>>>  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.
> 
> Yes, sorry, I didn't mean to suggest that the use of PKG_CHECK_MODULES
> rather than AC_CHECK_LIB was wrong.  But I think you can just pass
> similar if-found and if-not-found fragments.  Maybe something like:
> 
>  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.

>>>  Printing the errors to configure's terminal is
>>> not normally done, either.
>>
>> With this you mean the AC_MSG_WARN()?
> 
> I don't mind there being a call to AC_MSG_WARN.  I don't think I have
> a strong opinion about whether lack of zstd ought to produce a
> warning.  If there ought to be a warning, then it ought to be made
> with AC_MSG_WARN, indeed.
> 
> 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.

>> 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).
> 
> Does it ?  Admittedly the documentation I found in pkg.m4 for these
> PKG_* macros doesn't say what the default is for ACTION-IF-NOT-FOUND
> but it would surely parallel all the autoconf-provided macros where
> the default is a no-op.  I read the autoconf output in your patch
> (where admitteedly you pass [true]) and that seems to support my
> supposition.

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.

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

>>> 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
> 
> Oh!
> 
>> Hence the dependencies thereof also only ought to need
>> checking on x86.
> 
> I see.  Hmm.  TBH this seems anomalous.  I would prefer to keep the
> configure test and expect that eventually some non-x86 folsk will
> decide to turn this on there too.
> 
> 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.

>>>> +    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.
> 
> The endianness issue at least just means "this code doesn't work and
> will always reject images".  The alignment issue might mean "feeding
> a corrupted image file will crash your management daemon".
> 
> 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".

>> Since as per above this
>> code gets used on x86 only, I thought this would be fine at least
>> for now.
> 
> I think that's too much of a boobytrap to leave in the code.
> 
>> 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.
> 
> 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.

Jan

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.

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

I have a feeling we may be talking at cross purposes rather too much.

Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
> On 25.01.2021 15:53, Ian Jackson wrote:
> > 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 infer you're reading the autoonf output.  I think pkg_failed is
something to do with tracking whether pkg-config exists at all.  In
general, reading autoconf output is an act of desperation when RTFM
and so on fails.  The output is typically much more complicated than
the input and can be quite confusing.

I noticed that configure.ac fails to say PKG_PROG_PKG_CONFIG contrary
to the imprecations in the documentation.  For example, for
PKG_CHECK_MODULES we have:

 | # Note that if there is a possibility the first call to
 | # PKG_CHECK_MODULES might not happen, you should be sure to include
 | # an explicit call to PKG_PROG_PKG_CONFIG in your configure.ac
 
Indeed our first call to PKG_CHECK_* in the existing configure.ac is
within an if and there is no call to PKG_PROG_PKG_CONFIG.  I think one
should be added probably somewhere near the top (eg, just after
AX_XEN_EXPAND_CONFIG).

I'm not sure exactly what you mean in your paragraph I quote above.  I
think you mean that if the user supplies the options on the command
line bugt pkg-config is absent ?  I don't understand why this
situation should be handled differently for zstd than for any of the
other calls to *PKG* (glib, pixman, libnl).

Perhaps you experienced some issue which would have been fixed by the
addition of the missing PKG_PROG_PKG_CONFIG ?

(Also I note that the docs for PKG_CHECK_EXISTS contain a confusing
slip: there it says "you have to call PKG_CHECK_EXISTS manually" but
surely it must mean PKG_PROG_PKG_CONFIG.)

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

I don't understand what you are getting at.  I think you must have
misunderstood me.

You explained that you preferred not to use the 4th argument,
ACTION-IF-NOT-FOUND, "to avoid nesting".  I was trying to say that I
didn't think this was a good reason and that instead putting the code
in a separate conditional is not warranted here (and not idiomatic).

There is nothing wrong[1] with including (cautious) shell code in
configure.ac, so that was not part of my argument.

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

That would seem perfect to me.  I don't know what would be wrong
with it.

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

I think we had concluded not to print a warning ?

Ian.

[1] Contrary to the autoconf docs, which were written for a time when
even some very basic shell constructs such as "if" statements were not
always reliable.  In xen.git we can assume a vaguely posix shell.

On 25.01.2021 17:17, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
>> On 25.01.2021 15:53, Ian Jackson wrote:
>>> 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 infer you're reading the autoonf output.  I think pkg_failed is
> something to do with tracking whether pkg-config exists at all.  In
> general, reading autoconf output is an act of desperation when RTFM
> and so on fails.  The output is typically much more complicated than
> the input and can be quite confusing.

Well, after Michael's report I had to understand why the
construct behaved the way it does (and not the way I
thought would be sensible), and short of any documentation
clearly saying so I had to go look at the generated shell
code. Which made me notice the apparently (see below)
unhelpful behavior wrt user overrides.

> I noticed that configure.ac fails to say PKG_PROG_PKG_CONFIG contrary
> to the imprecations in the documentation.  For example, for
> PKG_CHECK_MODULES we have:
> 
>  | # Note that if there is a possibility the first call to
>  | # PKG_CHECK_MODULES might not happen, you should be sure to include
>  | # an explicit call to PKG_PROG_PKG_CONFIG in your configure.ac
>  
> Indeed our first call to PKG_CHECK_* in the existing configure.ac is
> within an if and there is no call to PKG_PROG_PKG_CONFIG.  I think one
> should be added probably somewhere near the top (eg, just after
> AX_XEN_EXPAND_CONFIG).

Probably, but I don't think I should do so here. I did ask
about making the compression checks x86-only, and that would
be the point where I would have seen the need. But you've
asked for the checks to remain arch-independent. 

> I'm not sure exactly what you mean in your paragraph I quote above.  I
> think you mean that if the user supplies the options on the command
> line bugt pkg-config is absent ?

Ah, looks like I indeed got mislead by the bad indentation
of the generate shell code. So let me try again with [true]
as the 4th argument.

>  I don't understand why this
> situation should be handled differently for zstd than for any of the
> other calls to *PKG* (glib, pixman, libnl).

The difference is that glib and pixman aren't optional (if
building qemu), i.e. we want configure to fail if they can't
be found or are too old.

> Perhaps you experienced some issue which would have been fixed by the
> addition of the missing PKG_PROG_PKG_CONFIG ?

I don't think so, no, as I've not tried configuring in a way
where the earlier PKG_CHECK_MODULES() would be bypassed.

>>>>> 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.
> 
> I don't understand what you are getting at.  I think you must have
> misunderstood me.
> 
> You explained that you preferred not to use the 4th argument,
> ACTION-IF-NOT-FOUND, "to avoid nesting".  I was trying to say that I
> didn't think this was a good reason and that instead putting the code
> in a separate conditional is not warranted here (and not idiomatic).
> 
> There is nothing wrong[1] with including (cautious) shell code in
> configure.ac, so that was not part of my argument.

I think the confusion results from my misunderstanding of when
"untried" would result, see above. For that reason I did
consider it necessary to evaluate things once _after_ the
entire construct, rather than inside.

>>>>>     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,
> 
> That would seem perfect to me.  I don't know what would be wrong
> with it.

Using this (most?) natural name has two issues in my view:
For one, it'll likely cause conflicts with how other code
(using hypervisor files) gets built. And then I consider it
odd to have just one out of a larger set of functions, but
I would consider it odd as well if I had to introduce them
all right here.

>>>>> 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).
> 
> I think we had concluded not to print a warning ?

Yes. Even in the projected new form of using the construct I
don't intend to change the description's wording, as the
intended use of [true] still looks like that can't be intended
usage. IOW my remark extended beyond the warning; I'm sorry if
this did end up confusing because you were referring to just
the warning.

Jan

The quoted-reply part of this message may be going off into the weeds.
Feel free to ignore it, or parts of it, if you think you can make
progress without disabusing me of what I think are my
misunderstandings...

Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
> On 25.01.2021 17:17, Ian Jackson wrote:
> >  I don't understand why this
> > situation should be handled differently for zstd than for any of the
> > other calls to *PKG* (glib, pixman, libnl).
> 
> The difference is that glib and pixman aren't optional (if
> building qemu), i.e. we want configure to fail if they can't
> be found or are too old.

Yes, but I think that just means adding the [true] fourth argument, to
make failure to find the library a no-op.  I don't think it needs any
more complex handling.  At least, I have not yet understood a need for
more complex handling...

> > Perhaps you experienced some issue which would have been fixed by the
> > addition of the missing PKG_PROG_PKG_CONFIG ?
> 
> I don't think so, no, as I've not tried configuring in a way
> where the earlier PKG_CHECK_MODULES() would be bypassed.

I guess I should take care of this then, since I think it's probably
an accident waiting to happen.

> >> I can, but it feels wrong, in particular if I gave it a
> >> generic looking name (get_unaligned_le32() or some such,
> > 
> > That would seem perfect to me.  I don't know what would be wrong
> > with it.
> 
> Using this (most?) natural name has two issues in my view:
> For one, it'll likely cause conflicts with how other code
> (using hypervisor files) gets built. And then I consider it
> odd to have just one out of a larger set of functions, but
> I would consider it odd as well if I had to introduce them
> all right here.

If you put this new definition in xg_private.h for now then it ought
not to conflict with anyone else.  (Assuming it's a static inline, or
a macro.  If it will have external linkage then it will need a name
prefix which I would prefer to avoid.)

I think it best to add this one macro/inline now.  When someone wants
more of these, they can add them.  If someone want them elsewhere they
can do the work of finding or making a suitably central place.  If it
weren't for the release timing I might think it better to add more,
but my general rule is that steps towards the best possible situation
are better than steps that go away from the best possible situation,
even if the former are not complete.

I think a reasonable alternative would be to arrange to import a
comprehensive set from somewhere.

> > I think we had concluded not to print a warning ?
> 
> Yes. Even in the projected new form of using the construct I
> don't intend to change the description's wording, as the
> intended use of [true] still looks like that can't be intended
> usage. IOW my remark extended beyond the warning; I'm sorry if
> this did end up confusing because you were referring to just
> the warning.

I'm afraid I don't understand what you mean.  In particular, what you
mean by "the intended use of [true] still looks like that can't be
intended usage".

  the intended {by whom for what puropose?} use of [true] still looks
  like that {what?} can't be intended {by whom?} usage

I have the feeling that I have totally failed to grasp your mental
model, which naturally underlies your comments.

Do you mean that with "true" for the 4th argument, the printed output
is not correct, in the failure case ?  Maybe it needs a call to AC_MSG
or something (but AIUI most of these PKG_* macros ought to do that for
us).  I'm just guessing at your meaing here...

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

Quoting this because I think it may still be relevant for
understanding the foregoing...

Ian.

On 25.01.2021 18:30, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
>> On 25.01.2021 17:17, Ian Jackson wrote:
>>> I think we had concluded not to print a warning ?
>>
>> Yes. Even in the projected new form of using the construct I
>> don't intend to change the description's wording, as the
>> intended use of [true] still looks like that can't be intended
>> usage. IOW my remark extended beyond the warning; I'm sorry if
>> this did end up confusing because you were referring to just
>> the warning.
> 
> I'm afraid I don't understand what you mean.  In particular, what you
> mean by "the intended use of [true] still looks like that can't be
> intended usage".
> 
>   the intended {by whom for what puropose?} use of [true] still looks
>   like that {what?} can't be intended {by whom?} usage
> 
> I have the feeling that I have totally failed to grasp your mental
> model, which naturally underlies your comments.
> 
> Do you mean that with "true" for the 4th argument, the printed output
> is not correct, in the failure case ?  Maybe it needs a call to AC_MSG
> or something (but AIUI most of these PKG_* macros ought to do that for
> us).  I'm just guessing at your meaing here...

Well, I'm afraid I'm ending up confusing you because I'm confused
about the possible intentions here. My initial attempt to avoid
configure failing was to specify [] as the 4th argument. This, to
me, would have felt the half-way natural indication that I don't
mean anything to be done in the failure case, neither autoconf's
default nor anything else. [true], otoh, already feels like a
workaround for some shortcoming.

Anyway - I guess we should continue from v3, which I hope to post
later this morning.

Jan

Jan Beulich writes ("Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels"):
> Anyway - I guess we should continue from v3, which I hope to post
> later this morning.

Thanks, yes.  I just wanted to reply to one bit of this:

>   My initial attempt to avoid configure failing was to specify [] as
> the 4th argument. This, to me, would have felt the half-way natural
> indication that I don't mean anything to be done in the failure
> case, neither autoconf's default nor anything else. [true], otoh,
> already feels like a workaround for some shortcoming.

configure.ac is m4 input text.  In m4, missing arguments to a macro
are experienced within the macro definition almost identically to an
empty argument.  (There are some GNU-m4-specific facilities that make
it possible for the macro to tell the difference, but it would be
highly unidiomatic to do that for an autoconf macro like this one.)

So naturally [] would do the same as a missing argument.  It's
possible that [ ] would have worked as well as [true] (depending on
the levels of quoting etc.), but [true] is more idiomatic to me.  It's
the usual way of spelling an explicit no-op in shell.

Ian.