[PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation

xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation
@ 2021-01-19 15:13 Jan Beulich
  2021-01-19 15:15 ` [PATCH v2 1/5] libxenguest: support zstd compressed kernels Jan Beulich
                   ` (5 more replies)
  0 siblings, 6 replies; 28+ messages in thread
From: Jan Beulich @ 2021-01-19 15:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu

Only the 1st patch is strictly intended for 4.15, paralleling
the recent Dom0 side work (and re-using some of the files
introduced there, for the stubdom build), but later ones may
still want considering.

1: libxenguest: support zstd compressed kernels
2: xen/decompress: make helper symbols static
3: libxenguest: "standardize" LZO kernel decompression code
4: libxenguest: drop redundant decompression declarations
5: libxenguest: simplify kernel decompression

Jan


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 1/5] libxenguest: support zstd compressed kernels
  2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
@ 2021-01-19 15:15 ` Jan Beulich
  2021-01-21 15:01   ` Wei Liu
  2021-01-19 15:15 ` [PATCH v2 2/5] xen/decompress: make helper symbols static Jan Beulich
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-19 15:15 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu

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.

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note for committer: As an alternative to patching tools/configure here,
autoconf may want re-running.
---
v2: New.

--- 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,97 @@ 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
+
+	as_fn_error $? "Package requirements (libzstd) were not met:
+
+$libzstd_PKG_ERRORS
+
+Consider adjusting the PKG_CONFIG_PATH environment variable if you
+installed software in a non-standard prefix.
+
+Alternatively, you may set the environment variables libzstd_CFLAGS
+and libzstd_LIBS to avoid the need to call pkg-config.
+See the pkg-config man page for more details." "$LINENO" 5
+elif test $pkg_failed = untried; then
+     	{ $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+	{ { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
+$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
+as_fn_error $? "The pkg-config script could not be found or is too old.  Make sure it
+is in your PATH or set the PKG_CONFIG environment variable to the full
+path to pkg-config.
+
+Alternatively, you may set the environment variables libzstd_CFLAGS
+and libzstd_LIBS to avoid the need to call pkg-config.
+See the pkg-config man page for more details.
+
+To get pkg-config, see <http://pkg-config.freedesktop.org/>.
+See \`config.log' for more details" "$LINENO" 5; }
+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; }
+	zlib="$zlib -DHAVE_ZSTD $libzstd_CFLAGS $libzstd_LIBS"
+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,7 @@ 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], [zlib="$zlib -DHAVE_ZSTD $libzstd_CFLAGS $libzstd_LIBS"])
 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



^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 2/5] xen/decompress: make helper symbols static
  2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
  2021-01-19 15:15 ` [PATCH v2 1/5] libxenguest: support zstd compressed kernels Jan Beulich
@ 2021-01-19 15:15 ` Jan Beulich
  2021-01-21 15:01   ` Wei Liu
  2021-01-19 15:16 ` [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code Jan Beulich
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-19 15:15 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu

The individual decompression CUs need to only surface their top level
functions to other code. Arrange for everything else to be static, to
make sure no undue uses of that code exist or will appear without
explicitly noticing. (In some cases this also results in code size
reduction, but since this is all init-only code this probably doesn't
matter very much.)

In the LZO case also take the opportunity and convert u8 where lines
get touched anyway.

The downside is that the top level functions will now be non-static
in stubdom builds of libxenguest, but I think that's acceptable. This
does require declaring them first, though, as the compiler warns about
the lack of declarations.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Fix stubdom build.

--- a/tools/libs/guest/xg_dom_decompress_unsafe.h
+++ b/tools/libs/guest/xg_dom_decompress_unsafe.h
@@ -1,8 +1,12 @@
+#ifdef __MINIOS__
+# include "../../xen/include/xen/decompress.h"
+#else
 typedef int decompress_fn(unsigned char *inbuf, unsigned int len,
                           int (*fill)(void*, unsigned int),
                           int (*flush)(void*, unsigned int),
                           unsigned char *outbuf, unsigned int *posp,
                           void (*error)(const char *x));
+#endif
 
 int xc_dom_decompress_unsafe(
     decompress_fn fn, struct xc_dom_image *dom, void **blob, size_t *size)
--- a/xen/common/bunzip2.c
+++ b/xen/common/bunzip2.c
@@ -665,12 +665,11 @@ static int INIT start_bunzip(struct bunz
 
 /* Example usage: decompress src_fd to dst_fd.  (Stops at end of bzip2 data,
    not end of file.) */
-STATIC int INIT bunzip2(unsigned char *buf, unsigned int len,
-			int(*fill)(void*, unsigned int),
-			int(*flush)(void*, unsigned int),
-			unsigned char *outbuf,
-			unsigned int *pos,
-			void(*error)(const char *x))
+int INIT bunzip2(unsigned char *buf, unsigned int len,
+		 int(*fill)(void*, unsigned int),
+		 int(*flush)(void*, unsigned int),
+		 unsigned char *outbuf, unsigned int *pos,
+		 void(*error)(const char *x))
 {
 	struct bunzip_data *bd;
 	int i = -1;
--- a/xen/common/decompress.h
+++ b/xen/common/decompress.h
@@ -7,7 +7,7 @@
 #include <xen/types.h>
 #include <xen/xmalloc.h>
 
-#define STATIC
+#define STATIC static
 #define INIT __init
 #define INITDATA __initdata
 
--- a/xen/common/unlz4.c
+++ b/xen/common/unlz4.c
@@ -22,12 +22,11 @@
 #define LZ4_DEFAULT_UNCOMPRESSED_CHUNK_SIZE (8 << 20)
 #define ARCHIVE_MAGICNUMBER 0x184C2102
 
-STATIC int INIT unlz4(unsigned char *input, unsigned int in_len,
-		      int (*fill)(void *, unsigned int),
-		      int (*flush)(void *, unsigned int),
-		      unsigned char *output,
-		      unsigned int *posp,
-		      void (*error)(const char *x))
+int INIT unlz4(unsigned char *input, unsigned int in_len,
+	       int (*fill)(void *, unsigned int),
+	       int (*flush)(void *, unsigned int),
+	       unsigned char *output, unsigned int *posp,
+	       void (*error)(const char *x))
 {
 	int ret = -1;
 	size_t chunksize = 0;
--- a/xen/common/unlzma.c
+++ b/xen/common/unlzma.c
@@ -528,13 +528,11 @@ static inline int INIT process_bit1(stru
 
 
 
-STATIC int INIT unlzma(unsigned char *buf, unsigned int in_len,
-		       int(*fill)(void*, unsigned int),
-		       int(*flush)(void*, unsigned int),
-		       unsigned char *output,
-		       unsigned int *posp,
-		       void(*error)(const char *x)
-	)
+int INIT unlzma(unsigned char *buf, unsigned int in_len,
+	        int(*fill)(void*, unsigned int),
+	        int(*flush)(void*, unsigned int),
+	        unsigned char *output, unsigned int *posp,
+	        void(*error)(const char *x))
 {
 	struct lzma_header header;
 	int lc, pb, lp;
--- a/xen/common/unlzo.c
+++ b/xen/common/unlzo.c
@@ -114,11 +114,11 @@ static int INIT parse_header(u8 *input,
 	return 1;
 }
 
-STATIC int INIT unlzo(u8 *input, unsigned int in_len,
-		      int (*fill) (void *, unsigned int),
-		      int (*flush) (void *, unsigned int),
-		      u8 *output, unsigned int *posp,
-		      void (*error) (const char *x))
+int INIT unlzo(unsigned char *input, unsigned int in_len,
+	       int (*fill) (void *, unsigned int),
+	       int (*flush) (void *, unsigned int),
+	       unsigned char *output, unsigned int *posp,
+	       void (*error) (const char *x))
 {
 	u8 r = 0;
 	int skip = 0;
--- a/xen/common/unxz.c
+++ b/xen/common/unxz.c
@@ -157,11 +157,11 @@
  * both input and output buffers are available as a single chunk, i.e. when
  * fill() and flush() won't be used.
  */
-STATIC int INIT unxz(unsigned char *in, unsigned int in_size,
-		     int (*fill)(void *dest, unsigned int size),
-		     int (*flush)(void *src, unsigned int size),
-		     unsigned char *out, unsigned int *in_used,
-		     void (*error)(const char *x))
+int INIT unxz(unsigned char *in, unsigned int in_size,
+	      int (*fill)(void *dest, unsigned int size),
+	      int (*flush)(void *src, unsigned int size),
+	      unsigned char *out, unsigned int *in_used,
+	      void (*error)(const char *x))
 {
 	struct xz_buf b;
 	struct xz_dec *s;
--- a/xen/common/unzstd.c
+++ b/xen/common/unzstd.c
@@ -142,12 +142,11 @@ out:
 	return err;
 }
 
-STATIC int INIT unzstd(unsigned char *in_buf, unsigned int in_len,
-		       int (*fill)(void*, unsigned int),
-		       int (*flush)(void*, unsigned int),
-		       unsigned char *out_buf,
-		       unsigned int *in_pos,
-		       void (*error)(const char *x))
+int INIT unzstd(unsigned char *in_buf, unsigned int in_len,
+	        int (*fill)(void*, unsigned int),
+	        int (*flush)(void*, unsigned int),
+	        unsigned char *out_buf, unsigned int *in_pos,
+	        void (*error)(const char *x))
 {
 	ZSTD_inBuffer in;
 	ZSTD_outBuffer out;



^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code
  2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
  2021-01-19 15:15 ` [PATCH v2 1/5] libxenguest: support zstd compressed kernels Jan Beulich
  2021-01-19 15:15 ` [PATCH v2 2/5] xen/decompress: make helper symbols static Jan Beulich
@ 2021-01-19 15:16 ` Jan Beulich
  2021-01-21 15:02   ` Wei Liu
  2021-01-19 15:16 ` [PATCH v2 4/5] libxenguest: drop redundant decompression declarations Jan Beulich
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-19 15:16 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu

Add a DOMPRINTF() other methods have, indicating success. To facilitate
this, introduce an "outsize" local variable and update *size as well as
*blob only once done. The latter then also avoids leaving a pointer to
freed memory in dom->kernel_blob in case of a decompression error.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/tools/libs/guest/xg_dom_bzimageloader.c
+++ b/tools/libs/guest/xg_dom_bzimageloader.c
@@ -409,7 +409,7 @@ static int xc_try_lzo1x_decode(
     int ret;
     const unsigned char *cur = dom->kernel_blob;
     unsigned char *out_buf = NULL;
-    size_t left = dom->kernel_size;
+    size_t left = dom->kernel_size, outsize;
     const char *msg;
     unsigned version;
     static const unsigned char magic[] = {
@@ -471,7 +471,7 @@ static int xc_try_lzo1x_decode(
     cur += ret;
     left -= ret;
 
-    for ( *size = 0; ; )
+    for ( outsize = 0; ; )
     {
         lzo_uint src_len, dst_len, out_len;
         unsigned char *tmp_buf;
@@ -484,9 +484,15 @@ static int xc_try_lzo1x_decode(
         if ( !dst_len )
         {
             msg = "Error registering stream output";
-            if ( xc_dom_register_external(dom, out_buf, *size) )
+            if ( xc_dom_register_external(dom, out_buf, outsize) )
                 break;
 
+            DOMPRINTF("%s: LZO decompress OK, 0x%zx -> 0x%zx",
+                      __FUNCTION__, *size, outsize);
+
+            *blob = out_buf;
+            *size = outsize;
+
             return 0;
         }
 
@@ -508,15 +514,15 @@ static int xc_try_lzo1x_decode(
             break;
 
         msg = "Output buffer overflow";
-        if ( *size > SIZE_MAX - dst_len )
+        if ( outsize > SIZE_MAX - dst_len )
             break;
 
         msg = "Decompressed image too large";
-        if ( xc_dom_kernel_check_size(dom, *size + dst_len) )
+        if ( xc_dom_kernel_check_size(dom, outsize + dst_len) )
             break;
 
         msg = "Failed to (re)alloc memory";
-        tmp_buf = realloc(out_buf, *size + dst_len);
+        tmp_buf = realloc(out_buf, outsize + dst_len);
         if ( tmp_buf == NULL )
             break;
 
@@ -524,7 +530,7 @@ static int xc_try_lzo1x_decode(
         out_len = dst_len;
 
         ret = lzo1x_decompress_safe(cur, src_len,
-                                    out_buf + *size, &out_len, NULL);
+                                    out_buf + outsize, &out_len, NULL);
         switch ( ret )
         {
         case LZO_E_OK:
@@ -532,8 +538,7 @@ static int xc_try_lzo1x_decode(
             if ( out_len != dst_len )
                 break;
 
-            *blob = out_buf;
-            *size += out_len;
+            outsize += out_len;
             cur += src_len;
             left -= src_len;
             continue;



^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 4/5] libxenguest: drop redundant decompression declarations
  2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
                   ` (2 preceding siblings ...)
  2021-01-19 15:16 ` [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code Jan Beulich
@ 2021-01-19 15:16 ` Jan Beulich
  2021-01-21 15:02   ` Wei Liu
  2021-01-19 15:17 ` [PATCH v2 5/5] libxenguest: simplify kernel decompression Jan Beulich
  2021-01-21 15:40 ` [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels Jan Beulich
  5 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-19 15:16 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu

The ones in xg_dom_decompress_unsafe.h suffice.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/tools/libs/guest/xg_dom_bzimageloader.c
+++ b/tools/libs/guest/xg_dom_bzimageloader.c
@@ -673,13 +673,6 @@ static int xc_try_zstd_decode(
 
 #endif
 
-#else /* __MINIOS__ */
-
-int xc_try_bzip2_decode(struct xc_dom_image *dom, void **blob, size_t *size);
-int xc_try_lzma_decode(struct xc_dom_image *dom, void **blob, size_t *size);
-int xc_try_lzo1x_decode(struct xc_dom_image *dom, void **blob, size_t *size);
-int xc_try_xz_decode(struct xc_dom_image *dom, void **blob, size_t *size);
-
 #endif /* !__MINIOS__ */
 
 struct setup_header {



^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 5/5] libxenguest: simplify kernel decompression
  2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
                   ` (3 preceding siblings ...)
  2021-01-19 15:16 ` [PATCH v2 4/5] libxenguest: drop redundant decompression declarations Jan Beulich
@ 2021-01-19 15:17 ` Jan Beulich
  2021-01-21 15:38   ` Wei Liu
  2021-01-21 15:40 ` [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels Jan Beulich
  5 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-19 15:17 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu

In all cases the kernel build makes available the uncompressed size in
the final 4 bytes of the bzImage payload. Utilize this to avoid
repeated realloc()ing of the output buffer.

As a side effect this also addresses the previous mistaken return of 0
(success) from xc_try_{bzip2,lzma,xz}_decode() in case
xc_dom_register_external() would have failed.

As another side effect this also addresses the first error path of
_xc_try_lzma_decode() previously bypassing lzma_end().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/tools/libs/guest/xg_dom_bzimageloader.c
+++ b/tools/libs/guest/xg_dom_bzimageloader.c
@@ -48,18 +48,16 @@ static int xc_try_bzip2_decode(
     bz_stream stream;
     int ret;
     char *out_buf;
-    char *tmp_buf;
     int retval = -1;
-    unsigned int outsize;
-    uint64_t total;
+    unsigned int insize, outsize;
 
     stream.bzalloc = NULL;
     stream.bzfree = NULL;
     stream.opaque = NULL;
 
-    if ( dom->kernel_size == 0)
+    if ( *size <= 8 )
     {
-        DOMPRINTF("BZIP2: Input is 0 size");
+        DOMPRINTF("BZIP2: insufficient input data");
         return -1;
     }
 
@@ -70,22 +68,25 @@ static int xc_try_bzip2_decode(
         return -1;
     }
 
-    /* sigh.  We don't know up-front how much memory we are going to need
-     * for the output buffer.  Allocate the output buffer to be equal
-     * the input buffer to start, and we'll realloc as needed.
-     */
-    outsize = dom->kernel_size;
+    insize = *size - 4;
+    outsize = *(uint32_t *)(*blob + insize);
 
     /*
-     * stream.avail_in and outsize are unsigned int, while kernel_size
+     * stream.avail_in and insize are unsigned int, while *size
      * is a size_t. Check we aren't overflowing.
      */
-    if ( outsize != dom->kernel_size )
+    if ( insize + 4 != *size )
     {
         DOMPRINTF("BZIP2: Input too large");
         goto bzip2_cleanup;
     }
 
+    if ( xc_dom_kernel_check_size(dom, outsize) )
+    {
+        DOMPRINTF("BZIP2: output too large");
+        goto bzip2_cleanup;
+    }
+
     out_buf = malloc(outsize);
     if ( out_buf == NULL )
     {
@@ -94,86 +95,45 @@ static int xc_try_bzip2_decode(
     }
 
     stream.next_in = dom->kernel_blob;
-    stream.avail_in = dom->kernel_size;
+    stream.avail_in = insize;
 
     stream.next_out = out_buf;
-    stream.avail_out = dom->kernel_size;
+    stream.avail_out = outsize;
 
-    for ( ; ; )
+    ret = BZ2_bzDecompress(&stream);
+    if ( ret == BZ_STREAM_END )
+        DOMPRINTF("BZIP2: Saw data stream end");
+    else if ( ret != BZ_OK )
     {
-        ret = BZ2_bzDecompress(&stream);
-        if ( ret == BZ_STREAM_END )
-        {
-            DOMPRINTF("BZIP2: Saw data stream end");
-            retval = 0;
-            break;
-        }
-        if ( ret != BZ_OK )
-        {
-            DOMPRINTF("BZIP2: error %d", ret);
-            free(out_buf);
-            goto bzip2_cleanup;
-        }
+        DOMPRINTF("BZIP2: error %d", ret);
+        free(out_buf);
+        goto bzip2_cleanup;
+    }
 
-        if ( stream.avail_out == 0 )
-        {
-            /* Protect against output buffer overflow */
-            if ( outsize > UINT_MAX / 2 )
-            {
-                DOMPRINTF("BZIP2: output buffer overflow");
-                free(out_buf);
-                goto bzip2_cleanup;
-            }
-
-            if ( xc_dom_kernel_check_size(dom, outsize * 2) )
-            {
-                DOMPRINTF("BZIP2: output too large");
-                free(out_buf);
-                goto bzip2_cleanup;
-            }
-
-            tmp_buf = realloc(out_buf, outsize * 2);
-            if ( tmp_buf == NULL )
-            {
-                DOMPRINTF("BZIP2: Failed to realloc memory");
-                free(out_buf);
-                goto bzip2_cleanup;
-            }
-            out_buf = tmp_buf;
-
-            stream.next_out = out_buf + outsize;
-            stream.avail_out = (outsize * 2) - outsize;
-            outsize *= 2;
-        }
-        else if ( stream.avail_in == 0 )
-        {
-            /*
-             * If there is output buffer available then this indicates
-             * that BZ2_bzDecompress would like more input data to be
-             * provided.  However our complete input buffer is in
-             * memory and provided upfront so if avail_in is zero this
-             * actually indicates a truncated input.
-             */
-            DOMPRINTF("BZIP2: not enough input");
-            free(out_buf);
-            goto bzip2_cleanup;
-        }
+    if ( stream.total_out_lo32 != outsize || stream.total_out_hi32 )
+    {
+        DOMPRINTF("BZIP2: got 0x%x%08x bytes instead of 0x%09x",
+                  stream.total_out_hi32, stream.total_out_lo32, outsize);
+        free(out_buf);
+        goto bzip2_cleanup;
     }
 
-    total = (((uint64_t)stream.total_out_hi32) << 32) | stream.total_out_lo32;
+    if ( stream.avail_in )
+        DOMPRINTF("BZIP2: Warning: %#x unconsumed bytes", stream.avail_in);
 
-    if ( xc_dom_register_external(dom, out_buf, total) )
+    if ( xc_dom_register_external(dom, out_buf, outsize) )
     {
         DOMPRINTF("BZIP2: Error registering stream output");
         free(out_buf);
         goto bzip2_cleanup;
     }
 
-    DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%lx",
-              __FUNCTION__, *size, (long unsigned int) total);
+    DOMPRINTF("%s: BZIP2 decompress OK, 0x%zx -> 0x%x",
+              __FUNCTION__, *size, outsize);
 
     *blob = out_buf;
-    *size = total;
+    *size = outsize;
+    retval = 0;
 
  bzip2_cleanup:
     BZ2_bzDecompressEnd(&stream);
@@ -205,22 +165,24 @@ static int _xc_try_lzma_decode(
     lzma_ret ret;
     lzma_action action = LZMA_RUN;
     unsigned char *out_buf;
-    unsigned char *tmp_buf;
     int retval = -1;
-    size_t outsize;
-    const char *msg;
+    size_t insize, outsize;
 
-    if ( dom->kernel_size == 0)
+    if ( *size < 8 )
     {
-        DOMPRINTF("%s: Input is 0 size", what);
-        return -1;
+        DOMPRINTF("%s: insufficient input data", what);
+        goto lzma_cleanup;
+    }
+
+    insize = *size - 4;
+    outsize = *(uint32_t *)(*blob + insize);
+
+    if ( xc_dom_kernel_check_size(dom, outsize) )
+    {
+        DOMPRINTF("%s: output too large", what);
+        goto lzma_cleanup;
     }
 
-    /* sigh.  We don't know up-front how much memory we are going to need
-     * for the output buffer.  Allocate the output buffer to be equal
-     * the input buffer to start, and we'll realloc as needed.
-     */
-    outsize = dom->kernel_size;
     out_buf = malloc(outsize);
     if ( out_buf == NULL )
     {
@@ -229,92 +191,68 @@ static int _xc_try_lzma_decode(
     }
 
     stream->next_in = dom->kernel_blob;
-    stream->avail_in = dom->kernel_size;
+    stream->avail_in = insize;
 
     stream->next_out = out_buf;
-    stream->avail_out = dom->kernel_size;
+    stream->avail_out = outsize;
 
-    for ( ; ; )
+    ret = lzma_code(stream, action);
+    if ( ret == LZMA_STREAM_END )
+        DOMPRINTF("%s: Saw data stream end", what);
+    else if ( ret != LZMA_OK )
     {
-        ret = lzma_code(stream, action);
-        if ( ret == LZMA_STREAM_END )
+        const char *msg;
+
+        switch ( ret )
         {
-            DOMPRINTF("%s: Saw data stream end", what);
-            retval = 0;
+        case LZMA_MEM_ERROR:
+            msg = strerror(ENOMEM);
             break;
-        }
-        if ( ret != LZMA_OK )
-        {
-            switch ( ret )
-            {
-            case LZMA_MEM_ERROR:
-                msg = strerror(ENOMEM);
-                break;
 
-            case LZMA_MEMLIMIT_ERROR:
-                msg = "Memory usage limit reached";
-                break;
+        case LZMA_MEMLIMIT_ERROR:
+            msg = "Memory usage limit reached";
+            break;
 
-            case LZMA_FORMAT_ERROR:
-                msg = "File format not recognized";
-                break;
+        case LZMA_FORMAT_ERROR:
+            msg = "File format not recognized";
+            break;
 
-            case LZMA_OPTIONS_ERROR:
-                // FIXME: Better message?
-                msg = "Unsupported compression options";
-                break;
+        case LZMA_OPTIONS_ERROR:
+            // FIXME: Better message?
+            msg = "Unsupported compression options";
+            break;
 
-            case LZMA_DATA_ERROR:
-                msg = "File is corrupt";
-                break;
+        case LZMA_DATA_ERROR:
+            msg = "File is corrupt";
+            break;
 
-            case LZMA_BUF_ERROR:
-                msg = "Unexpected end of input";
-                break;
+        case LZMA_BUF_ERROR:
+            msg = "Unexpected end of input";
+            break;
 
-            default:
-                msg = "Internal program error (bug)";
-                break;
-            }
-            DOMPRINTF("%s: %s decompression error: %s",
-                      __FUNCTION__, what, msg);
-            free(out_buf);
-            goto lzma_cleanup;
+         default:
+            msg = "Internal program error (bug)";
+            break;
         }
 
-        if ( stream->avail_out == 0 )
-        {
-            /* Protect against output buffer overflow */
-            if ( outsize > SIZE_MAX / 2 )
-            {
-                DOMPRINTF("%s: output buffer overflow", what);
-                free(out_buf);
-                goto lzma_cleanup;
-            }
-
-            if ( xc_dom_kernel_check_size(dom, outsize * 2) )
-            {
-                DOMPRINTF("%s: output too large", what);
-                free(out_buf);
-                goto lzma_cleanup;
-            }
-
-            tmp_buf = realloc(out_buf, outsize * 2);
-            if ( tmp_buf == NULL )
-            {
-                DOMPRINTF("%s: Failed to realloc memory", what);
-                free(out_buf);
-                goto lzma_cleanup;
-            }
-            out_buf = tmp_buf;
-
-            stream->next_out = out_buf + outsize;
-            stream->avail_out = (outsize * 2) - outsize;
-            outsize *= 2;
-        }
+        DOMPRINTF("%s: %s decompression error: %s",
+                  __FUNCTION__, what, msg);
+        free(out_buf);
+        goto lzma_cleanup;
+    }
+
+    if ( stream->total_out != outsize )
+    {
+        DOMPRINTF("%s: got 0x%"PRIx64" bytes instead of 0x%zx",
+                  what, stream->total_out, outsize);
+        free(out_buf);
+        goto lzma_cleanup;
     }
 
-    if ( xc_dom_register_external(dom, out_buf, stream->total_out) )
+    if ( stream->avail_in )
+        DOMPRINTF("%s: Warning: %#zx unconsumed bytes", what, stream->avail_in);
+
+    if ( xc_dom_register_external(dom, out_buf, outsize) )
     {
         DOMPRINTF("%s: Error registering stream output", what);
         free(out_buf);
@@ -322,10 +260,11 @@ static int _xc_try_lzma_decode(
     }
 
     DOMPRINTF("%s: %s decompress OK, 0x%zx -> 0x%zx",
-              __FUNCTION__, what, *size, (size_t)stream->total_out);
+              __FUNCTION__, what, *size, outsize);
 
     *blob = out_buf;
-    *size = stream->total_out;
+    *size = outsize;
+    retval = 0;
 
  lzma_cleanup:
     lzma_end(stream);
@@ -408,8 +347,8 @@ static int xc_try_lzo1x_decode(
 {
     int ret;
     const unsigned char *cur = dom->kernel_blob;
-    unsigned char *out_buf = NULL;
-    size_t left = dom->kernel_size, outsize;
+    unsigned char *out_buf;
+    size_t left = dom->kernel_size, outsize, outtot;
     const char *msg;
     unsigned version;
     static const unsigned char magic[] = {
@@ -435,6 +374,15 @@ static int xc_try_lzo1x_decode(
         return -1;
     }
 
+    left -= 4;
+    outtot = *(uint32_t *)(*blob + left);
+
+    if ( xc_dom_kernel_check_size(dom, outtot) )
+    {
+        DOMPRINTF("LZO1x: output too large");
+        return -1;
+    }
+
     /* get version (2bytes), skip library version (2),
      * 'need to be extracted' version (2) and method (1) */
     version = lzo_read_16(cur + 9);
@@ -471,10 +419,16 @@ static int xc_try_lzo1x_decode(
     cur += ret;
     left -= ret;
 
+    out_buf = malloc(outtot);
+    if ( !out_buf )
+    {
+        DOMPRINTF("LZO1x: failed to alloc memory");
+        return -1;
+    }
+
     for ( outsize = 0; ; )
     {
         lzo_uint src_len, dst_len, out_len;
-        unsigned char *tmp_buf;
 
         msg = "Short input";
         if ( left < 4 )
@@ -483,6 +437,13 @@ static int xc_try_lzo1x_decode(
         dst_len = lzo_read_32(cur);
         if ( !dst_len )
         {
+            msg = "Unexpected output size";
+            if ( outsize != outtot )
+                break;
+
+            if ( left != 4 )
+                DOMPRINTF("LZO1x: Warning: %#zx unconsumed bytes", left - 4);
+
             msg = "Error registering stream output";
             if ( xc_dom_register_external(dom, out_buf, outsize) )
                 break;
@@ -514,19 +475,9 @@ static int xc_try_lzo1x_decode(
             break;
 
         msg = "Output buffer overflow";
-        if ( outsize > SIZE_MAX - dst_len )
-            break;
-
-        msg = "Decompressed image too large";
-        if ( xc_dom_kernel_check_size(dom, outsize + dst_len) )
-            break;
-
-        msg = "Failed to (re)alloc memory";
-        tmp_buf = realloc(out_buf, outsize + dst_len);
-        if ( tmp_buf == NULL )
+        if ( dst_len > outtot - outsize )
             break;
 
-        out_buf = tmp_buf;
         out_len = dst_len;
 
         ret = lzo1x_decompress_safe(cur, src_len,



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 1/5] libxenguest: support zstd compressed kernels
  2021-01-19 15:15 ` [PATCH v2 1/5] libxenguest: support zstd compressed kernels Jan Beulich
@ 2021-01-21 15:01   ` Wei Liu
  2021-01-21 15:05     ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Wei Liu @ 2021-01-21 15:01 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini, Wei Liu

On Tue, Jan 19, 2021 at 04:15:25PM +0100, Jan Beulich wrote:
> 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.
> 
> 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.

Noted. FWIW I use Debian 10 to generate configure. If I don't get around
doing it someone who runs the same system should be able to help.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 2/5] xen/decompress: make helper symbols static
  2021-01-19 15:15 ` [PATCH v2 2/5] xen/decompress: make helper symbols static Jan Beulich
@ 2021-01-21 15:01   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2021-01-21 15:01 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini, Wei Liu

On Tue, Jan 19, 2021 at 04:15:53PM +0100, Jan Beulich wrote:
> The individual decompression CUs need to only surface their top level
> functions to other code. Arrange for everything else to be static, to
> make sure no undue uses of that code exist or will appear without
> explicitly noticing. (In some cases this also results in code size
> reduction, but since this is all init-only code this probably doesn't
> matter very much.)
> 
> In the LZO case also take the opportunity and convert u8 where lines
> get touched anyway.
> 
> The downside is that the top level functions will now be non-static
> in stubdom builds of libxenguest, but I think that's acceptable. This
> does require declaring them first, though, as the compiler warns about
> the lack of declarations.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wl@xen.org>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code
  2021-01-19 15:16 ` [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code Jan Beulich
@ 2021-01-21 15:02   ` Wei Liu
  2021-01-25 11:59     ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Wei Liu @ 2021-01-21 15:02 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini, Wei Liu

On Tue, Jan 19, 2021 at 04:16:35PM +0100, Jan Beulich wrote:
> Add a DOMPRINTF() other methods have, indicating success. To facilitate
> this, introduce an "outsize" local variable and update *size as well as
> *blob only once done. The latter then also avoids leaving a pointer to
> freed memory in dom->kernel_blob in case of a decompression error.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Wei Liu <wl@xen.org>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 4/5] libxenguest: drop redundant decompression declarations
  2021-01-19 15:16 ` [PATCH v2 4/5] libxenguest: drop redundant decompression declarations Jan Beulich
@ 2021-01-21 15:02   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2021-01-21 15:02 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini, Wei Liu

On Tue, Jan 19, 2021 at 04:16:53PM +0100, Jan Beulich wrote:
> The ones in xg_dom_decompress_unsafe.h suffice.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Wei Liu <wl@xen.org>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 1/5] libxenguest: support zstd compressed kernels
  2021-01-21 15:01   ` Wei Liu
@ 2021-01-21 15:05     ` Jan Beulich
  2021-01-21 15:33       ` Wei Liu
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-21 15:05 UTC (permalink / raw)
  To: Wei Liu
  Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini

On 21.01.2021 16:01, Wei Liu wrote:
> On Tue, Jan 19, 2021 at 04:15:25PM +0100, Jan Beulich wrote:
>> 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.
>>
>> 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>

Thanks, but I'm about to send v2.5 to address the issue reported
by Michael Young (adjusting configure{.ac,} only).

>> ---
>> Note for committer: As an alternative to patching tools/configure here,
>> autoconf may want re-running.
> 
> Noted. FWIW I use Debian 10 to generate configure. If I don't get around
> doing it someone who runs the same system should be able to help.

Well, the version I've been using to re-generate isn't identical
to yours, but allows easily filtering out and discarding the few
extra differences, which is why I dared to provide a diff for
tools/configure in the first place. IOW if I would commit this
myself, all I would ask that you or someone else using the
"original" autoconf to generate the committed version to double
check.

Jan


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 1/5] libxenguest: support zstd compressed kernels
  2021-01-21 15:05     ` Jan Beulich
@ 2021-01-21 15:33       ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2021-01-21 15:33 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Liu, xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini

On Thu, Jan 21, 2021 at 04:05:39PM +0100, Jan Beulich wrote:
> On 21.01.2021 16:01, Wei Liu wrote:
> > On Tue, Jan 19, 2021 at 04:15:25PM +0100, Jan Beulich wrote:
> >> 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.
> >>
> >> 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>
> 
> Thanks, but I'm about to send v2.5 to address the issue reported
> by Michael Young (adjusting configure{.ac,} only).
> 
> >> ---
> >> Note for committer: As an alternative to patching tools/configure here,
> >> autoconf may want re-running.
> > 
> > Noted. FWIW I use Debian 10 to generate configure. If I don't get around
> > doing it someone who runs the same system should be able to help.
> 
> Well, the version I've been using to re-generate isn't identical
> to yours, but allows easily filtering out and discarding the few
> extra differences, which is why I dared to provide a diff for
> tools/configure in the first place. IOW if I would commit this
> myself, all I would ask that you or someone else using the
> "original" autoconf to generate the committed version to double
> check.

This works too.

Wei.

> 
> Jan


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 5/5] libxenguest: simplify kernel decompression
  2021-01-19 15:17 ` [PATCH v2 5/5] libxenguest: simplify kernel decompression Jan Beulich
@ 2021-01-21 15:38   ` Wei Liu
  0 siblings, 0 replies; 28+ messages in thread
From: Wei Liu @ 2021-01-21 15:38 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
	Julien Grall, Stefano Stabellini, Wei Liu

On Tue, Jan 19, 2021 at 04:17:12PM +0100, Jan Beulich wrote:
> In all cases the kernel build makes available the uncompressed size in
> the final 4 bytes of the bzImage payload. Utilize this to avoid
> repeated realloc()ing of the output buffer.
> 
> As a side effect this also addresses the previous mistaken return of 0
> (success) from xc_try_{bzip2,lzma,xz}_decode() in case
> xc_dom_register_external() would have failed.
> 
> As another side effect this also addresses the first error path of
> _xc_try_lzma_decode() previously bypassing lzma_end().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I think the code changes are correct:

Acked-by: Wei Liu <wl@xen.org>

But a second pair of eyes would be useful here since this patch is
complex.

Wei.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
                   ` (4 preceding siblings ...)
  2021-01-19 15:17 ` [PATCH v2 5/5] libxenguest: simplify kernel decompression Jan Beulich
@ 2021-01-21 15:40 ` Jan Beulich
  2021-01-25 11:30   ` Ian Jackson
  5 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-21 15:40 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-21 15:40 ` [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels Jan Beulich
@ 2021-01-25 11:30   ` Ian Jackson
  2021-01-25 12:42     ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 11:30 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code
  2021-01-21 15:02   ` Wei Liu
@ 2021-01-25 11:59     ` Ian Jackson
  2021-01-25 12:45       ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 11:59 UTC (permalink / raw)
  To: Wei Liu
  Cc: Jan Beulich, xen-devel, Andrew Cooper, George Dunlap,
	Julien Grall, Stefano Stabellini

Wei Liu writes ("Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code"):
> On Tue, Jan 19, 2021 at 04:16:35PM +0100, Jan Beulich wrote:
> > Add a DOMPRINTF() other methods have, indicating success. To facilitate
> > this, introduce an "outsize" local variable and update *size as well as
> > *blob only once done. The latter then also avoids leaving a pointer to
> > freed memory in dom->kernel_blob in case of a decompression error.
> > 
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Wei Liu <wl@xen.org>

The latter part of this is a bugfix which ought to go into 4.15, I
think, and be backported.

I don't mind throwing in the DOMPRINTF too.

Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 11:30   ` Ian Jackson
@ 2021-01-25 12:42     ` Jan Beulich
  2021-01-25 13:51       ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-25 12:42 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code
  2021-01-25 11:59     ` Ian Jackson
@ 2021-01-25 12:45       ` Jan Beulich
  2021-01-25 13:30         ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-25 12:45 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu

On 25.01.2021 12:59, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code"):
>> On Tue, Jan 19, 2021 at 04:16:35PM +0100, Jan Beulich wrote:
>>> Add a DOMPRINTF() other methods have, indicating success. To facilitate
>>> this, introduce an "outsize" local variable and update *size as well as
>>> *blob only once done. The latter then also avoids leaving a pointer to
>>> freed memory in dom->kernel_blob in case of a decompression error.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Acked-by: Wei Liu <wl@xen.org>
> 
> The latter part of this is a bugfix which ought to go into 4.15, I
> think, and be backported.
> 
> I don't mind throwing in the DOMPRINTF too.

Am I fine to transliterate this into R-a-b?

Jan


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code
  2021-01-25 12:45       ` Jan Beulich
@ 2021-01-25 13:30         ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 13:30 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu

Jan Beulich writes ("Re: [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code"):
> On 25.01.2021 12:59, Ian Jackson wrote:
> > I don't mind throwing in the DOMPRINTF too.
> 
> Am I fine to transliterate this into R-a-b?

Err, yes, sorry, should have been more explicit.

Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 12:42     ` Jan Beulich
@ 2021-01-25 13:51       ` Ian Jackson
  2021-01-25 14:30         ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 13:51 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Ian Jackson, xen-devel, Andrew Cooper, George Dunlap,
	Julien Grall, Stefano Stabellini, Wei Liu, M A Young

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 13:51       ` Ian Jackson
@ 2021-01-25 14:30         ` Jan Beulich
  2021-01-25 14:53           ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-25 14:30 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 14:30         ` Jan Beulich
@ 2021-01-25 14:53           ` Ian Jackson
  2021-01-25 15:31             ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 14:53 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 14:53           ` Ian Jackson
@ 2021-01-25 15:31             ` Jan Beulich
  2021-01-25 16:17               ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-25 15:31 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 15:31             ` Jan Beulich
@ 2021-01-25 16:17               ` Ian Jackson
  2021-01-25 17:00                 ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 16:17 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 16:17               ` Ian Jackson
@ 2021-01-25 17:00                 ` Jan Beulich
  2021-01-25 17:30                   ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-25 17:00 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 17:00                 ` Jan Beulich
@ 2021-01-25 17:30                   ` Ian Jackson
  2021-01-26  7:47                     ` Jan Beulich
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2021-01-25 17:30 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-25 17:30                   ` Ian Jackson
@ 2021-01-26  7:47                     ` Jan Beulich
  2021-01-26 12:14                       ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-01-26  7:47 UTC (permalink / raw)
  To: Ian Jackson
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels
  2021-01-26  7:47                     ` Jan Beulich
@ 2021-01-26 12:14                       ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2021-01-26 12:14 UTC (permalink / raw)
  To: Jan Beulich
  Cc: xen-devel, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, M A Young

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.

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2021-01-26 12:15 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 15:13 [PATCH v2 0/5] zstd decompression for DomU-s + fallout / consolidation Jan Beulich
2021-01-19 15:15 ` [PATCH v2 1/5] libxenguest: support zstd compressed kernels Jan Beulich
2021-01-21 15:01   ` Wei Liu
2021-01-21 15:05     ` Jan Beulich
2021-01-21 15:33       ` Wei Liu
2021-01-19 15:15 ` [PATCH v2 2/5] xen/decompress: make helper symbols static Jan Beulich
2021-01-21 15:01   ` Wei Liu
2021-01-19 15:16 ` [PATCH v2 3/5] libxenguest: "standardize" LZO kernel decompression code Jan Beulich
2021-01-21 15:02   ` Wei Liu
2021-01-25 11:59     ` Ian Jackson
2021-01-25 12:45       ` Jan Beulich
2021-01-25 13:30         ` Ian Jackson
2021-01-19 15:16 ` [PATCH v2 4/5] libxenguest: drop redundant decompression declarations Jan Beulich
2021-01-21 15:02   ` Wei Liu
2021-01-19 15:17 ` [PATCH v2 5/5] libxenguest: simplify kernel decompression Jan Beulich
2021-01-21 15:38   ` Wei Liu
2021-01-21 15:40 ` [PATCH v2.5 1/5] libxenguest: support zstd compressed kernels Jan Beulich
2021-01-25 11:30   ` Ian Jackson
2021-01-25 12:42     ` Jan Beulich
2021-01-25 13:51       ` Ian Jackson
2021-01-25 14:30         ` Jan Beulich
2021-01-25 14:53           ` Ian Jackson
2021-01-25 15:31             ` Jan Beulich
2021-01-25 16:17               ` Ian Jackson
2021-01-25 17:00                 ` Jan Beulich
2021-01-25 17:30                   ` Ian Jackson
2021-01-26  7:47                     ` Jan Beulich
2021-01-26 12:14                       ` Ian Jackson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).