| Message ID | 20191018101835.2264070-1-tolga.hosgor@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add Zstd compression support | expand |
Hi Tolga, On 18/10/19 12:18, Hosgor, Tolga (IOT DS EU TR MTS) wrote: > From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga.hosgor@siemens.com> > > Build-time configuration to support Zstd decompression of raw compressed > images. > > Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) <tolga.hosgor@siemens.com> > --- > Kconfig | 19 ++++++++- > Makefile.deps | 4 ++ > Makefile.flags | 4 ++ > core/cpio_utils.c | 99 +++++++++++++++++++++++++++++++++++++++++++---- > 4 files changed, 116 insertions(+), 10 deletions(-) > > diff --git a/Kconfig b/Kconfig > index 882ee1e..e9c535d 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -61,6 +61,10 @@ config HAVE_ZLIB > bool > option env="HAVE_ZLIB" > > +config HAVE_ZSTD > + bool > + option env="HAVE_ZSTD" > + > config HAVE_LIBSSL > bool > option env="HAVE_LIBSSL" > @@ -441,10 +445,21 @@ source suricatta/Config.in > > source mongoose/Config.in > > +choice > + prompt "Compression implementation to use" > + default GUNZIP > + help > + Select compression implementation for decompressing images. > + > config GUNZIP > - bool > + bool "GUNZIP" > depends on HAVE_ZLIB > - default y Dropping default will create incompatibility. GUNZIP should be the default compressor. > + > +config ZSTD > + bool "ZSTD" > + depends on HAVE_ZSTD > + > +endchoice Well, do you really need to exclude one of the compressor ? Why ? My thought about the feature is to have multiple compressors and they can be detected automatically (mmhh..this can add unwanted complexity) or selected in sw-description. We can have compressed = true; compression-type = "zstd"; and if compression-type is not set, it falls back to "zlib". > > source parser/Config.in > source handlers/Config.in > diff --git a/Makefile.deps b/Makefile.deps > index 512d4b8..5e2bd2f 100644 > --- a/Makefile.deps > +++ b/Makefile.deps > @@ -38,6 +38,10 @@ ifeq ($(HAVE_ZLIB),) > export HAVE_ZLIB = y > endif > > +ifeq ($(HAVE_ZSTD),) > +export HAVE_ZSTD = y > +endif > + > ifeq ($(HAVE_LIBUBOOTENV),) > export HAVE_LIBUBOOTENV = y > endif > diff --git a/Makefile.flags b/Makefile.flags > index 0edd48c..38db720 100644 > --- a/Makefile.flags > +++ b/Makefile.flags > @@ -162,6 +162,10 @@ ifeq ($(CONFIG_GUNZIP),y) > LDLIBS += z > endif > > +ifeq ($(CONFIG_ZSTD),y) > +LDLIBS += zstd > +endif > + > ifeq ($(CONFIG_RDIFFHANDLER),y) > LDLIBS += rsync > endif > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index 1b6cb14..9332a4c 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -10,8 +10,10 @@ > #include <stdio.h> > #include <unistd.h> > #include <errno.h> > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) > #include <zlib.h> > +#elif defined(CONFIG_ZSTD) > +#include <zstd.h> > #endif > > #include "generated/autoconf.h" > @@ -233,7 +235,7 @@ static int decrypt_step(void *state, void *buffer, size_t size) > return 0; > } > > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) > > struct GunzipState > { > @@ -283,6 +285,55 @@ static int gunzip_step(void *state, void *buffer, size_t size) > return outlen; > } > > +#elif defined(CONFIG_ZSTD) > + > +struct ZstdState > +{ > + PipelineStep upstream_step; > + void *upstream_state; > + > + ZSTD_DStream* dctx; > + ZSTD_inBuffer input; > + uint8_t input_buffer[BUFF_SIZE]; > + bool eof; > +}; > + > +static int zstd_step(void* state, void* buffer, size_t size) > +{ > + struct ZstdState *s = (struct ZstdState *)state; > + size_t decompress_ret; > + int ret; > + ZSTD_outBuffer output = { buffer, size, 0 }; > + > + do { > + if (s->input.pos == s->input.size) { > + ret = s->upstream_step(s->upstream_state, s->input_buffer, sizeof s->input_buffer); > + if (ret < 0) { > + return ret; > + } else if (ret == 0) { > + s->eof = true; > + } > + s->input.size = ret; > + s->input.pos = 0; > + } > + > + do { > + decompress_ret = ZSTD_decompressStream(s->dctx, &output, &s->input); > + > + if (ZSTD_isError(decompress_ret)) { > + ERROR("ZSTD_decompressStream failed (returned %zu)", decompress_ret); > + return -1; > + } > + > + if (output.pos == output.size) { > + break; > + } > + } while (s->input.pos < s->input.size); > + } while (output.pos == 0 && !s->eof); > + > + return output.pos; > +} > + > #endif > > int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsigned long long seek, > @@ -314,7 +365,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > .outlen = 0, .eof = false > }; > > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) > struct GunzipState gunzip_state = { > .upstream_step = NULL, .upstream_state = NULL, > .strm = { > @@ -325,6 +376,13 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > .initialized = false, > .eof = false > }; > +#elif defined(CONFIG_ZSTD) > + struct ZstdState zstd_state = { > + .upstream_step = NULL, .upstream_state = NULL, > + .dctx = NULL, > + .input = { NULL, 0, 0 }, > + .eof = false > + }; > #endif > > PipelineStep step = NULL; > @@ -356,7 +414,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > } > > if (compressed) { > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) > /* > * 16 + MAX_WBITS means that Zlib should expect and decode a > * gzip header. > @@ -367,8 +425,15 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > goto copyfile_exit; > } > gunzip_state.initialized = true; > +#elif defined(CONFIG_ZSTD) > + if ((zstd_state.dctx = ZSTD_createDStream()) == NULL) { > + ERROR("ZSTD_createDStream failed"); > + ret = -EFAULT; > + goto copyfile_exit; > + } > + zstd_state.input.src = zstd_state.input_buffer; > #else > - TRACE("Request decompressing, but CONFIG_GUNZIP not set !"); > + TRACE("Request decompressing, but CONFIG_GUNZIP or CONFIG_ZSTD not set !"); > ret = -EINVAL; > goto copyfile_exit; > #endif > @@ -384,7 +449,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > } > } > > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) > if (compressed) { > if (encrypted) { > decrypt_state.upstream_step = &input_step; > @@ -398,6 +463,20 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > step = &gunzip_step; > state = &gunzip_state; > } else { > +#elif defined(CONFIG_ZSTD) > + if (compressed) { > + if (encrypted) { > + decrypt_state.upstream_step = &input_step; > + decrypt_state.upstream_state = &input_state; > + zstd_state.upstream_step = &decrypt_step; > + zstd_state.upstream_state = &decrypt_state; > + } else { > + zstd_state.upstream_step = &input_step; > + zstd_state.upstream_state = &input_state; > + } > + step = &zstd_step; > + state = &zstd_state; > + } else { > #endif Because the interface is exactly the same as for zlib, I would not like to see this. Let's say we define a structure with upstream step and state, and this is added here to have like: if (compressed) { if (encrypted) { decrypt_state.upstream_step = &input_step; decrypt_state.upstream_state = &input_state; compress_state.upstream_step = &decrypt_step; compress_state.upstream_state = &decrypt_state; } else { compress_state.upstream_step = &input_step; compress_state.upstream_state = &input_state; } where compress_state (and a compress_step) was set before to zstd or gunzip, depending on the chosen compressor. > if (encrypted) { > decrypt_state.upstream_step = &input_step; > @@ -408,7 +487,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi > step = &input_step; > state = &input_state; > } > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) || defined(CONFIG_ZSTD) > } > #endif > > @@ -482,10 +561,14 @@ copyfile_exit: > if (input_state.dgst) { > swupdate_HASH_cleanup(input_state.dgst); > } > -#ifdef CONFIG_GUNZIP > +#if defined(CONFIG_GUNZIP) > if (gunzip_state.initialized) { > inflateEnd(&gunzip_state.strm); > } > +#elif defined(CONFIG_ZSTD) > + if (zstd_state.dctx != NULL) { > + ZSTD_freeDStream(zstd_state.dctx); > + } > #endif > > return ret; > Best regards, Stefano Babic
On Fri, 2019-10-18 at 13:18 +0200, Stefano Babic wrote: > Hi Tolga, > > On 18/10/19 12:18, Hosgor, Tolga (IOT DS EU TR MTS) wrote: > > From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga.hosgor@siemens.com> > > > > Build-time configuration to support Zstd decompression of raw > > compressed > > images. > > > > Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) < > > tolga.hosgor@siemens.com> > > --- > > Kconfig | 19 ++++++++- > > Makefile.deps | 4 ++ > > Makefile.flags | 4 ++ > > core/cpio_utils.c | 99 > > +++++++++++++++++++++++++++++++++++++++++++---- > > 4 files changed, 116 insertions(+), 10 deletions(-) > > > > diff --git a/Kconfig b/Kconfig > > index 882ee1e..e9c535d 100644 > > --- a/Kconfig > > +++ b/Kconfig > > @@ -61,6 +61,10 @@ config HAVE_ZLIB > > bool > > option env="HAVE_ZLIB" > > > > +config HAVE_ZSTD > > + bool > > + option env="HAVE_ZSTD" > > + > > config HAVE_LIBSSL > > bool > > option env="HAVE_LIBSSL" > > @@ -441,10 +445,21 @@ source suricatta/Config.in > > > > source mongoose/Config.in > > > > +choice > > + prompt "Compression implementation to use" > > + default GUNZIP > > + help > > + Select compression implementation for decompressing images. > > + > > config GUNZIP > > - bool > > + bool "GUNZIP" > > depends on HAVE_ZLIB > > - default y > > Dropping default will create incompatibility. GUNZIP should be the > default compressor. I have done it in 'choice' section. +choice + prompt "Compression implementation to use" + default GUNZIP I am not super familiar with kconfig. I can correct it if this is wrong. > > > + > > +config ZSTD > > + bool "ZSTD" > > + depends on HAVE_ZSTD > > + > > +endchoice > > Well, do you really need to exclude one of the compressor ? Why ? > > My thought about the feature is to have multiple compressors and they > can be detected automatically (mmhh..this can add unwanted > complexity) > or selected in sw-description. We can have > > compressed = true; > compression-type = "zstd"; > > and if compression-type is not set, it falls back to "zlib". I thought about this automatic detection but it would be unnecessarily complex like you said. I like the idea of a "compression-type". I could and should rework this to support that probably. > > > > > source parser/Config.in > > source handlers/Config.in > > diff --git a/Makefile.deps b/Makefile.deps > > index 512d4b8..5e2bd2f 100644 > > --- a/Makefile.deps > > +++ b/Makefile.deps > > @@ -38,6 +38,10 @@ ifeq ($(HAVE_ZLIB),) > > export HAVE_ZLIB = y > > endif > > > > +ifeq ($(HAVE_ZSTD),) > > +export HAVE_ZSTD = y > > +endif > > + > > ifeq ($(HAVE_LIBUBOOTENV),) > > export HAVE_LIBUBOOTENV = y > > endif > > diff --git a/Makefile.flags b/Makefile.flags > > index 0edd48c..38db720 100644 > > --- a/Makefile.flags > > +++ b/Makefile.flags > > @@ -162,6 +162,10 @@ ifeq ($(CONFIG_GUNZIP),y) > > LDLIBS += z > > endif > > > > +ifeq ($(CONFIG_ZSTD),y) > > +LDLIBS += zstd > > +endif > > + > > ifeq ($(CONFIG_RDIFFHANDLER),y) > > LDLIBS += rsync > > endif > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > > index 1b6cb14..9332a4c 100644 > > --- a/core/cpio_utils.c > > +++ b/core/cpio_utils.c > > @@ -10,8 +10,10 @@ > > #include <stdio.h> > > #include <unistd.h> > > #include <errno.h> > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) > > #include <zlib.h> > > +#elif defined(CONFIG_ZSTD) > > +#include <zstd.h> > > #endif > > > > #include "generated/autoconf.h" > > @@ -233,7 +235,7 @@ static int decrypt_step(void *state, void > > *buffer, size_t size) > > return 0; > > } > > > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) > > > > struct GunzipState > > { > > @@ -283,6 +285,55 @@ static int gunzip_step(void *state, void > > *buffer, size_t size) > > return outlen; > > } > > > > +#elif defined(CONFIG_ZSTD) > > + > > +struct ZstdState > > +{ > > + PipelineStep upstream_step; > > + void *upstream_state; > > + > > + ZSTD_DStream* dctx; > > + ZSTD_inBuffer input; > > + uint8_t input_buffer[BUFF_SIZE]; > > + bool eof; > > +}; > > + > > +static int zstd_step(void* state, void* buffer, size_t size) > > +{ > > + struct ZstdState *s = (struct ZstdState *)state; > > + size_t decompress_ret; > > + int ret; > > + ZSTD_outBuffer output = { buffer, size, 0 }; > > + > > + do { > > + if (s->input.pos == s->input.size) { > > + ret = s->upstream_step(s->upstream_state, s- > > >input_buffer, sizeof s->input_buffer); > > + if (ret < 0) { > > + return ret; > > + } else if (ret == 0) { > > + s->eof = true; > > + } > > + s->input.size = ret; > > + s->input.pos = 0; > > + } > > + > > + do { > > + decompress_ret = ZSTD_decompressStream(s->dctx, > > &output, &s->input); > > + > > + if (ZSTD_isError(decompress_ret)) { > > + ERROR("ZSTD_decompressStream failed > > (returned %zu)", decompress_ret); > > + return -1; > > + } > > + > > + if (output.pos == output.size) { > > + break; > > + } > > + } while (s->input.pos < s->input.size); > > + } while (output.pos == 0 && !s->eof); > > + > > + return output.pos; > > +} > > + > > #endif > > > > int copyfile(int fdin, void *out, unsigned int nbytes, unsigned > > long *offs, unsigned long long seek, > > @@ -314,7 +365,7 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > .outlen = 0, .eof = false > > }; > > > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) > > struct GunzipState gunzip_state = { > > .upstream_step = NULL, .upstream_state = NULL, > > .strm = { > > @@ -325,6 +376,13 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > .initialized = false, > > .eof = false > > }; > > +#elif defined(CONFIG_ZSTD) > > + struct ZstdState zstd_state = { > > + .upstream_step = NULL, .upstream_state = NULL, > > + .dctx = NULL, > > + .input = { NULL, 0, 0 }, > > + .eof = false > > + }; > > #endif > > > > PipelineStep step = NULL; > > @@ -356,7 +414,7 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > } > > > > if (compressed) { > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) > > /* > > * 16 + MAX_WBITS means that Zlib should expect and > > decode a > > * gzip header. > > @@ -367,8 +425,15 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > goto copyfile_exit; > > } > > gunzip_state.initialized = true; > > +#elif defined(CONFIG_ZSTD) > > + if ((zstd_state.dctx = ZSTD_createDStream()) == NULL) { > > + ERROR("ZSTD_createDStream failed"); > > + ret = -EFAULT; > > + goto copyfile_exit; > > + } > > + zstd_state.input.src = zstd_state.input_buffer; > > #else > > - TRACE("Request decompressing, but CONFIG_GUNZIP not set > > !"); > > + TRACE("Request decompressing, but CONFIG_GUNZIP or > > CONFIG_ZSTD not set !"); > > ret = -EINVAL; > > goto copyfile_exit; > > #endif > > @@ -384,7 +449,7 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > } > > } > > > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) > > if (compressed) { > > if (encrypted) { > > decrypt_state.upstream_step = &input_step; > > @@ -398,6 +463,20 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > step = &gunzip_step; > > state = &gunzip_state; > > } else { > > +#elif defined(CONFIG_ZSTD) > > + if (compressed) { > > + if (encrypted) { > > + decrypt_state.upstream_step = &input_step; > > + decrypt_state.upstream_state = &input_state; > > + zstd_state.upstream_step = &decrypt_step; > > + zstd_state.upstream_state = &decrypt_state; > > + } else { > > + zstd_state.upstream_step = &input_step; > > + zstd_state.upstream_state = &input_state; > > + } > > + step = &zstd_step; > > + state = &zstd_state; > > + } else { > > #endif > > Because the interface is exactly the same as for zlib, I would not > like > to see this. Let's say we define a structure with upstream step and > state, and this is added here to have like: > > if (compressed) { > if (encrypted) { > decrypt_state.upstream_step = &input_step; > decrypt_state.upstream_state = &input_state; > compress_state.upstream_step = &decrypt_step; > compress_state.upstream_state = &decrypt_state; > } else { > compress_state.upstream_step = &input_step; > compress_state.upstream_state = &input_state; > } > > where compress_state (and a compress_step) was set before to zstd or > gunzip, depending on the chosen compressor. > Yeah that part definitely should be changed. I wanted to keep this part explicit for the first iteration as I also do not like the mutually exclusive gunzip/zstd anyway. > > > if (encrypted) { > > decrypt_state.upstream_step = &input_step; > > @@ -408,7 +487,7 @@ int copyfile(int fdin, void *out, unsigned int > > nbytes, unsigned long *offs, unsi > > step = &input_step; > > state = &input_state; > > } > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) || defined(CONFIG_ZSTD) > > } > > #endif > > > > @@ -482,10 +561,14 @@ copyfile_exit: > > if (input_state.dgst) { > > swupdate_HASH_cleanup(input_state.dgst); > > } > > -#ifdef CONFIG_GUNZIP > > +#if defined(CONFIG_GUNZIP) > > if (gunzip_state.initialized) { > > inflateEnd(&gunzip_state.strm); > > } > > +#elif defined(CONFIG_ZSTD) > > + if (zstd_state.dctx != NULL) { > > + ZSTD_freeDStream(zstd_state.dctx); > > + } > > #endif > > > > return ret; > > Thanks for the input. Let me try this compression-type approach then. > > Best regards, > Stefano Babic >
On 18/10/19 13:29, Tolga HOŞGÖR wrote: > On Fri, 2019-10-18 at 13:18 +0200, Stefano Babic wrote: >> Hi Tolga, >> >> On 18/10/19 12:18, Hosgor, Tolga (IOT DS EU TR MTS) wrote: >>> From: Hosgor, Tolga (IOT DS EU TR MTS) <tolga.hosgor@siemens.com> >>> >>> Build-time configuration to support Zstd decompression of raw >>> compressed >>> images. >>> >>> Signed-off-by: Hosgor, Tolga (IOT DS EU TR MTS) < >>> tolga.hosgor@siemens.com> >>> --- >>> Kconfig | 19 ++++++++- >>> Makefile.deps | 4 ++ >>> Makefile.flags | 4 ++ >>> core/cpio_utils.c | 99 >>> +++++++++++++++++++++++++++++++++++++++++++---- >>> 4 files changed, 116 insertions(+), 10 deletions(-) >>> >>> diff --git a/Kconfig b/Kconfig >>> index 882ee1e..e9c535d 100644 >>> --- a/Kconfig >>> +++ b/Kconfig >>> @@ -61,6 +61,10 @@ config HAVE_ZLIB >>> bool >>> option env="HAVE_ZLIB" >>> >>> +config HAVE_ZSTD >>> + bool >>> + option env="HAVE_ZSTD" >>> + >>> config HAVE_LIBSSL >>> bool >>> option env="HAVE_LIBSSL" >>> @@ -441,10 +445,21 @@ source suricatta/Config.in >>> >>> source mongoose/Config.in >>> >>> +choice >>> + prompt "Compression implementation to use" >>> + default GUNZIP >>> + help >>> + Select compression implementation for decompressing images. >>> + >>> config GUNZIP >>> - bool >>> + bool "GUNZIP" >>> depends on HAVE_ZLIB >>> - default y >> >> Dropping default will create incompatibility. GUNZIP should be the >> default compressor. > > I have done it in 'choice' section. > > +choice > + prompt "Compression implementation to use" > + default GUNZIP > > I am not super familiar with kconfig. I can correct it if this is > wrong. Ah, ok, you are right. Anyway, as "choice", just one of the compressors can be chosen. >> >>> + >>> +config ZSTD >>> + bool "ZSTD" >>> + depends on HAVE_ZSTD >>> + >>> +endchoice >> >> Well, do you really need to exclude one of the compressor ? Why ? >> >> My thought about the feature is to have multiple compressors and they >> can be detected automatically (mmhh..this can add unwanted >> complexity) >> or selected in sw-description. We can have >> >> compressed = true; >> compression-type = "zstd"; >> >> and if compression-type is not set, it falls back to "zlib". > I thought about this automatic detection but it would be unnecessarily > complex like you said. Yes, agree. We can forget it. I guess it will be unnecessary: sw-description must be correct in all its parts, not only for compression. A wrong compressor is then like any other errors in sw-description, and SWUpdate will stop with an error. This is ok IMHO. > > I like the idea of a "compression-type". I could and should rework this > to support that probably. Right. >> >>> >>> source parser/Config.in >>> source handlers/Config.in >>> diff --git a/Makefile.deps b/Makefile.deps >>> index 512d4b8..5e2bd2f 100644 >>> --- a/Makefile.deps >>> +++ b/Makefile.deps >>> @@ -38,6 +38,10 @@ ifeq ($(HAVE_ZLIB),) >>> export HAVE_ZLIB = y >>> endif >>> >>> +ifeq ($(HAVE_ZSTD),) >>> +export HAVE_ZSTD = y >>> +endif >>> + >>> ifeq ($(HAVE_LIBUBOOTENV),) >>> export HAVE_LIBUBOOTENV = y >>> endif >>> diff --git a/Makefile.flags b/Makefile.flags >>> index 0edd48c..38db720 100644 >>> --- a/Makefile.flags >>> +++ b/Makefile.flags >>> @@ -162,6 +162,10 @@ ifeq ($(CONFIG_GUNZIP),y) >>> LDLIBS += z >>> endif >>> >>> +ifeq ($(CONFIG_ZSTD),y) >>> +LDLIBS += zstd >>> +endif >>> + >>> ifeq ($(CONFIG_RDIFFHANDLER),y) >>> LDLIBS += rsync >>> endif >>> diff --git a/core/cpio_utils.c b/core/cpio_utils.c >>> index 1b6cb14..9332a4c 100644 >>> --- a/core/cpio_utils.c >>> +++ b/core/cpio_utils.c >>> @@ -10,8 +10,10 @@ >>> #include <stdio.h> >>> #include <unistd.h> >>> #include <errno.h> >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) >>> #include <zlib.h> >>> +#elif defined(CONFIG_ZSTD) >>> +#include <zstd.h> >>> #endif >>> >>> #include "generated/autoconf.h" >>> @@ -233,7 +235,7 @@ static int decrypt_step(void *state, void >>> *buffer, size_t size) >>> return 0; >>> } >>> >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) >>> >>> struct GunzipState >>> { >>> @@ -283,6 +285,55 @@ static int gunzip_step(void *state, void >>> *buffer, size_t size) >>> return outlen; >>> } >>> >>> +#elif defined(CONFIG_ZSTD) >>> + >>> +struct ZstdState >>> +{ >>> + PipelineStep upstream_step; >>> + void *upstream_state; >>> + >>> + ZSTD_DStream* dctx; >>> + ZSTD_inBuffer input; >>> + uint8_t input_buffer[BUFF_SIZE]; >>> + bool eof; >>> +}; >>> + >>> +static int zstd_step(void* state, void* buffer, size_t size) >>> +{ >>> + struct ZstdState *s = (struct ZstdState *)state; >>> + size_t decompress_ret; >>> + int ret; >>> + ZSTD_outBuffer output = { buffer, size, 0 }; >>> + >>> + do { >>> + if (s->input.pos == s->input.size) { >>> + ret = s->upstream_step(s->upstream_state, s- >>>> input_buffer, sizeof s->input_buffer); >>> + if (ret < 0) { >>> + return ret; >>> + } else if (ret == 0) { >>> + s->eof = true; >>> + } >>> + s->input.size = ret; >>> + s->input.pos = 0; >>> + } >>> + >>> + do { >>> + decompress_ret = ZSTD_decompressStream(s->dctx, >>> &output, &s->input); >>> + >>> + if (ZSTD_isError(decompress_ret)) { >>> + ERROR("ZSTD_decompressStream failed >>> (returned %zu)", decompress_ret); >>> + return -1; >>> + } >>> + >>> + if (output.pos == output.size) { >>> + break; >>> + } >>> + } while (s->input.pos < s->input.size); >>> + } while (output.pos == 0 && !s->eof); >>> + >>> + return output.pos; >>> +} >>> + >>> #endif >>> >>> int copyfile(int fdin, void *out, unsigned int nbytes, unsigned >>> long *offs, unsigned long long seek, >>> @@ -314,7 +365,7 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> .outlen = 0, .eof = false >>> }; >>> >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) >>> struct GunzipState gunzip_state = { >>> .upstream_step = NULL, .upstream_state = NULL, >>> .strm = { >>> @@ -325,6 +376,13 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> .initialized = false, >>> .eof = false >>> }; >>> +#elif defined(CONFIG_ZSTD) >>> + struct ZstdState zstd_state = { >>> + .upstream_step = NULL, .upstream_state = NULL, >>> + .dctx = NULL, >>> + .input = { NULL, 0, 0 }, >>> + .eof = false >>> + }; >>> #endif >>> >>> PipelineStep step = NULL; >>> @@ -356,7 +414,7 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> } >>> >>> if (compressed) { >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) >>> /* >>> * 16 + MAX_WBITS means that Zlib should expect and >>> decode a >>> * gzip header. >>> @@ -367,8 +425,15 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> goto copyfile_exit; >>> } >>> gunzip_state.initialized = true; >>> +#elif defined(CONFIG_ZSTD) >>> + if ((zstd_state.dctx = ZSTD_createDStream()) == NULL) { >>> + ERROR("ZSTD_createDStream failed"); >>> + ret = -EFAULT; >>> + goto copyfile_exit; >>> + } >>> + zstd_state.input.src = zstd_state.input_buffer; >>> #else >>> - TRACE("Request decompressing, but CONFIG_GUNZIP not set >>> !"); >>> + TRACE("Request decompressing, but CONFIG_GUNZIP or >>> CONFIG_ZSTD not set !"); >>> ret = -EINVAL; >>> goto copyfile_exit; >>> #endif >>> @@ -384,7 +449,7 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> } >>> } >>> >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) >>> if (compressed) { >>> if (encrypted) { >>> decrypt_state.upstream_step = &input_step; >>> @@ -398,6 +463,20 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> step = &gunzip_step; >>> state = &gunzip_state; >>> } else { >>> +#elif defined(CONFIG_ZSTD) >>> + if (compressed) { >>> + if (encrypted) { >>> + decrypt_state.upstream_step = &input_step; >>> + decrypt_state.upstream_state = &input_state; >>> + zstd_state.upstream_step = &decrypt_step; >>> + zstd_state.upstream_state = &decrypt_state; >>> + } else { >>> + zstd_state.upstream_step = &input_step; >>> + zstd_state.upstream_state = &input_state; >>> + } >>> + step = &zstd_step; >>> + state = &zstd_state; >>> + } else { >>> #endif >> >> Because the interface is exactly the same as for zlib, I would not >> like >> to see this. Let's say we define a structure with upstream step and >> state, and this is added here to have like: >> >> if (compressed) { >> if (encrypted) { >> decrypt_state.upstream_step = &input_step; >> decrypt_state.upstream_state = &input_state; >> compress_state.upstream_step = &decrypt_step; >> compress_state.upstream_state = &decrypt_state; >> } else { >> compress_state.upstream_step = &input_step; >> compress_state.upstream_state = &input_state; >> } >> >> where compress_state (and a compress_step) was set before to zstd or >> gunzip, depending on the chosen compressor. >> > Yeah that part definitely should be changed. I wanted to keep this part > explicit for the first iteration as I also do not like the mutually > exclusive gunzip/zstd anyway. Ok, understood. >> >>> if (encrypted) { >>> decrypt_state.upstream_step = &input_step; >>> @@ -408,7 +487,7 @@ int copyfile(int fdin, void *out, unsigned int >>> nbytes, unsigned long *offs, unsi >>> step = &input_step; >>> state = &input_state; >>> } >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) || defined(CONFIG_ZSTD) >>> } >>> #endif >>> >>> @@ -482,10 +561,14 @@ copyfile_exit: >>> if (input_state.dgst) { >>> swupdate_HASH_cleanup(input_state.dgst); >>> } >>> -#ifdef CONFIG_GUNZIP >>> +#if defined(CONFIG_GUNZIP) >>> if (gunzip_state.initialized) { >>> inflateEnd(&gunzip_state.strm); >>> } >>> +#elif defined(CONFIG_ZSTD) >>> + if (zstd_state.dctx != NULL) { >>> + ZSTD_freeDStream(zstd_state.dctx); >>> + } >>> #endif >>> >>> return ret; >>> > Thanks for the input. Let me try this compression-type approach then. You have also not added any single line to the documentation. If we add multiple compressors, this must be exhaustive *documented*. Best regards, Stefano
diff --git a/Kconfig b/Kconfig index 882ee1e..e9c535d 100644 --- a/Kconfig +++ b/Kconfig @@ -61,6 +61,10 @@ config HAVE_ZLIB bool option env="HAVE_ZLIB" +config HAVE_ZSTD + bool + option env="HAVE_ZSTD" + config HAVE_LIBSSL bool option env="HAVE_LIBSSL" @@ -441,10 +445,21 @@ source suricatta/Config.in source mongoose/Config.in +choice + prompt "Compression implementation to use" + default GUNZIP + help + Select compression implementation for decompressing images. + config GUNZIP - bool + bool "GUNZIP" depends on HAVE_ZLIB - default y + +config ZSTD + bool "ZSTD" + depends on HAVE_ZSTD + +endchoice source parser/Config.in source handlers/Config.in diff --git a/Makefile.deps b/Makefile.deps index 512d4b8..5e2bd2f 100644 --- a/Makefile.deps +++ b/Makefile.deps @@ -38,6 +38,10 @@ ifeq ($(HAVE_ZLIB),) export HAVE_ZLIB = y endif +ifeq ($(HAVE_ZSTD),) +export HAVE_ZSTD = y +endif + ifeq ($(HAVE_LIBUBOOTENV),) export HAVE_LIBUBOOTENV = y endif diff --git a/Makefile.flags b/Makefile.flags index 0edd48c..38db720 100644 --- a/Makefile.flags +++ b/Makefile.flags @@ -162,6 +162,10 @@ ifeq ($(CONFIG_GUNZIP),y) LDLIBS += z endif +ifeq ($(CONFIG_ZSTD),y) +LDLIBS += zstd +endif + ifeq ($(CONFIG_RDIFFHANDLER),y) LDLIBS += rsync endif diff --git a/core/cpio_utils.c b/core/cpio_utils.c index 1b6cb14..9332a4c 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -10,8 +10,10 @@ #include <stdio.h> #include <unistd.h> #include <errno.h> -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) #include <zlib.h> +#elif defined(CONFIG_ZSTD) +#include <zstd.h> #endif #include "generated/autoconf.h" @@ -233,7 +235,7 @@ static int decrypt_step(void *state, void *buffer, size_t size) return 0; } -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) struct GunzipState { @@ -283,6 +285,55 @@ static int gunzip_step(void *state, void *buffer, size_t size) return outlen; } +#elif defined(CONFIG_ZSTD) + +struct ZstdState +{ + PipelineStep upstream_step; + void *upstream_state; + + ZSTD_DStream* dctx; + ZSTD_inBuffer input; + uint8_t input_buffer[BUFF_SIZE]; + bool eof; +}; + +static int zstd_step(void* state, void* buffer, size_t size) +{ + struct ZstdState *s = (struct ZstdState *)state; + size_t decompress_ret; + int ret; + ZSTD_outBuffer output = { buffer, size, 0 }; + + do { + if (s->input.pos == s->input.size) { + ret = s->upstream_step(s->upstream_state, s->input_buffer, sizeof s->input_buffer); + if (ret < 0) { + return ret; + } else if (ret == 0) { + s->eof = true; + } + s->input.size = ret; + s->input.pos = 0; + } + + do { + decompress_ret = ZSTD_decompressStream(s->dctx, &output, &s->input); + + if (ZSTD_isError(decompress_ret)) { + ERROR("ZSTD_decompressStream failed (returned %zu)", decompress_ret); + return -1; + } + + if (output.pos == output.size) { + break; + } + } while (s->input.pos < s->input.size); + } while (output.pos == 0 && !s->eof); + + return output.pos; +} + #endif int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsigned long long seek, @@ -314,7 +365,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi .outlen = 0, .eof = false }; -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) struct GunzipState gunzip_state = { .upstream_step = NULL, .upstream_state = NULL, .strm = { @@ -325,6 +376,13 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi .initialized = false, .eof = false }; +#elif defined(CONFIG_ZSTD) + struct ZstdState zstd_state = { + .upstream_step = NULL, .upstream_state = NULL, + .dctx = NULL, + .input = { NULL, 0, 0 }, + .eof = false + }; #endif PipelineStep step = NULL; @@ -356,7 +414,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi } if (compressed) { -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) /* * 16 + MAX_WBITS means that Zlib should expect and decode a * gzip header. @@ -367,8 +425,15 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi goto copyfile_exit; } gunzip_state.initialized = true; +#elif defined(CONFIG_ZSTD) + if ((zstd_state.dctx = ZSTD_createDStream()) == NULL) { + ERROR("ZSTD_createDStream failed"); + ret = -EFAULT; + goto copyfile_exit; + } + zstd_state.input.src = zstd_state.input_buffer; #else - TRACE("Request decompressing, but CONFIG_GUNZIP not set !"); + TRACE("Request decompressing, but CONFIG_GUNZIP or CONFIG_ZSTD not set !"); ret = -EINVAL; goto copyfile_exit; #endif @@ -384,7 +449,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi } } -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) if (compressed) { if (encrypted) { decrypt_state.upstream_step = &input_step; @@ -398,6 +463,20 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi step = &gunzip_step; state = &gunzip_state; } else { +#elif defined(CONFIG_ZSTD) + if (compressed) { + if (encrypted) { + decrypt_state.upstream_step = &input_step; + decrypt_state.upstream_state = &input_state; + zstd_state.upstream_step = &decrypt_step; + zstd_state.upstream_state = &decrypt_state; + } else { + zstd_state.upstream_step = &input_step; + zstd_state.upstream_state = &input_state; + } + step = &zstd_step; + state = &zstd_state; + } else { #endif if (encrypted) { decrypt_state.upstream_step = &input_step; @@ -408,7 +487,7 @@ int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs, unsi step = &input_step; state = &input_state; } -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) || defined(CONFIG_ZSTD) } #endif @@ -482,10 +561,14 @@ copyfile_exit: if (input_state.dgst) { swupdate_HASH_cleanup(input_state.dgst); } -#ifdef CONFIG_GUNZIP +#if defined(CONFIG_GUNZIP) if (gunzip_state.initialized) { inflateEnd(&gunzip_state.strm); } +#elif defined(CONFIG_ZSTD) + if (zstd_state.dctx != NULL) { + ZSTD_freeDStream(zstd_state.dctx); + } #endif return ret;