Re: [Crash-utility] [PATCH v2] diskdump: Add support for reading dumpfiles compressed by Zstandard

Hi Kazu, the patch follows the snappy and lzo example and all in all the looks good to me. Two small comments below though. Nevertheless Reviewed-by: Philipp Rudo <prudo(a)redhat.com&gt;

On Fri, 17 Sep 2021 07:55:32 +0000 HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab(a)nec.com&gt; wrote: > -----Original Message----- > > @@ -1002,6 +1006,11 @@ is_diskdump(char *file) > > dd->flags |= SNAPPY_SUPPORTED; > > #endif > > > > +#ifdef ZSTD > > + if ((dctx = ZSTD_createDCtx()) != NULL) > > + dd->flags |= ZSTD_SUPPORTED; > > I found that this part allocates dctx superfluously for split dumpfiles, > so sending v2. > > -- > From: Kazuhito Hagio <k-hagio(a)ab.jp.nec.com&gt; > Subject: [PATCH] diskdump: Add support for reading dumpfiles compressed by > Zstandard > > Add support for reading dumpfiles compressed by Zstandard (zstd) > using makedumpfile. > > To build crash with zstd support, type "make zstd". > > Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; > --- > Makefile | 4 ++++ > README | 4 ++-- > configure.c | 20 +++++++++++++++++--- > defs.h | 4 ++++ > diskdump.c | 33 +++++++++++++++++++++++++++++++++ > diskdump.h | 1 + > help.c | 4 ++-- > 7 files changed, 63 insertions(+), 7 deletions(-) [...] > --- a/configure.c > +++ b/configure.c > @@ -1746,6 +1746,7 @@ add_extra_lib(char *option) You should probably add a 'For ZSTD:' section in the comment above this function as well.

> { > int lzo, add_DLZO, add_llzo2; > int snappy, add_DSNAPPY, add_lsnappy; > + int zstd, add_DZSTD, add_lzstd; > int valgrind, add_DVALGRIND; > char *cflags, *ldflags; > FILE *fp_cflags, *fp_ldflags; [...] > --- a/diskdump.c > +++ b/diskdump.c > @@ -96,6 +96,10 @@ static struct diskdump_data **dd_list = NULL; > static int num_dd = 0; > static int num_dumpfiles = 0; > > +#ifdef ZSTD > +static ZSTD_DCtx *dctx = NULL; > +#endif > + > int dumpfile_is_split(void) > { > return KDUMP_SPLIT(); > @@ -1002,6 +1006,13 @@ is_diskdump(char *file) > dd->flags |= SNAPPY_SUPPORTED; > #endif > > +#ifdef ZSTD > + if (!dctx) > + dctx = ZSTD_createDCtx(); > + if (dctx) > + dd->flags |= ZSTD_SUPPORTED; > +#endif > + would it make sense to only set ZSTD_SUPPORTED here and move the creation of dctx to cache_page where it is used? That at least would save a couple of bytes when zstd is supported but the dump is not zstd compressed.

Thanks Philipp > pc->read_vmcoreinfo = vmcoreinfo_read_string; > > if ((pc->flags2 & GET_LOG) && KDUMP_CMPRS_VALID()) { > @@ -1251,6 +1262,24 @@ cache_page(physaddr_t paddr) > ret); > return READ_ERROR; > } > +#endif > + } else if (pd.flags & DUMP_DH_COMPRESSED_ZSTD) { > + > + if (!(dd->flags & ZSTD_SUPPORTED)) { > + error(INFO, "%s: uncompess failed: no zstd compression support\n", > + DISKDUMP_VALID() ? "diskdump" : "compressed kdump"); > + return READ_ERROR; > + } > +#ifdef ZSTD > + retlen = ZSTD_decompressDCtx(dctx, > + dd->page_cache_hdr[i].pg_bufptr, block_size, > + dd->compressed_page, pd.size); > + if (ZSTD_isError(retlen)) { > + error(INFO, "%s: uncompress failed: %d (%s)\n", > + DISKDUMP_VALID() ? "diskdump" : "compressed kdump", > + retlen, ZSTD_getErrorName(retlen)); > + return READ_ERROR; > + } > #endif > } else > memcpy(dd->page_cache_hdr[i].pg_bufptr, > @@ -1806,6 +1835,8 @@ __diskdump_memory_dump(FILE *fp) > fprintf(fp, "%sLZO_SUPPORTED", others++ ? "|" : ""); > if (dd->flags & SNAPPY_SUPPORTED) > fprintf(fp, "%sSNAPPY_SUPPORTED", others++ ? "|" : ""); > + if (dd->flags & ZSTD_SUPPORTED) > + fprintf(fp, "%sZSTD_SUPPORTED", others++ ? "|" : ""); > fprintf(fp, ") %s\n", FLAT_FORMAT() ? "[FLAT]" : ""); > fprintf(fp, " dfd: %d\n", dd->dfd); > fprintf(fp, " ofp: %lx\n", (ulong)dd->ofp); > @@ -1872,6 +1903,8 @@ __diskdump_memory_dump(FILE *fp) > fprintf(fp, "DUMP_DH_COMPRESSED_LZO"); > if (dh->status & DUMP_DH_COMPRESSED_SNAPPY) > fprintf(fp, "DUMP_DH_COMPRESSED_SNAPPY"); > + if (dh->status & DUMP_DH_COMPRESSED_ZSTD) > + fprintf(fp, "DUMP_DH_COMPRESSED_ZSTD"); > if (dh->status & DUMP_DH_COMPRESSED_INCOMPLETE) > fprintf(fp, "DUMP_DH_COMPRESSED_INCOMPLETE"); > if (dh->status & DUMP_DH_EXCLUDED_VMEMMAP) > diff --git a/diskdump.h b/diskdump.h > index 28713407b841..c152c7b86616 100644 > --- a/diskdump.h > +++ b/diskdump.h > @@ -86,6 +86,7 @@ struct kdump_sub_header { > #define DUMP_DH_COMPRESSED_SNAPPY 0x4 /* page is compressed with snappy */ > #define DUMP_DH_COMPRESSED_INCOMPLETE 0x8 /* dumpfile is incomplete */ > #define DUMP_DH_EXCLUDED_VMEMMAP 0x10 /* unused vmemmap pages are excluded */ > +#define DUMP_DH_COMPRESSED_ZSTD 0x20 /* page is compressed with zstd */ > > /* descriptor of each page for vmcore */ > typedef struct page_desc { > diff --git a/help.c b/help.c > index 6c262a3ffcbb..f34838d59908 100644 > --- a/help.c > +++ b/help.c > @@ -9420,8 +9420,8 @@ README_ENTER_DIRECTORY, > " Traditionally when vmcores are compressed via the makedumpfile(8) facility", > " the libz compression library is used, and by default the crash utility", > " only supports libz. Recently makedumpfile has been enhanced to optionally", > -" use either the LZO or snappy compression libraries. To build crash with", > -" either or both of those libraries, type \"make lzo\" or \"make snappy\".", > +" use the LZO, snappy or zstd compression libraries. To build crash with any", > +" or all of those libraries, type \"make lzo\", \"make snappy\" or \"make zstd\".", > "", > " crash supports valgrind Memcheck tool on the crash's custom memory allocator.", > " To build crash with this feature enabled, type \"make valgrind\" and then run",