[PATCH 0/5] trace-cmd: Improvements in compression logic

[PATCH 0/5] trace-cmd: Improvements in compression logic

[Tzvetomir Stoyanov (VMware)]

Improvements in internal comperssion APIs, suggested by
Sebastian Andrzej Siewior.

Tzvetomir Stoyanov (VMware) (5):
  trace-cmd: Use a structure to describe a compression protocol
  trace-cmd: Make internal compression hooks more generic
  trace-cmd: Use errno from zlib, if available
  trace-cmd: Add context to compression hooks
  trace-cmd: Use context hooks in zstd

 .../include/private/trace-cmd-private.h       |  20 ++--
 lib/trace-cmd/trace-compress-zlib.c           |  41 ++++---
 lib/trace-cmd/trace-compress-zstd.c           | 100 ++++++++++++------
 lib/trace-cmd/trace-compress.c                |  80 +++++++-------
 4 files changed, 144 insertions(+), 97 deletions(-)

-- 
2.34.1

[PATCH 1/5] trace-cmd: Use a structure to describe a compression protocol

[Tzvetomir Stoyanov (VMware)]

Changed the tracecmd_compress_proto_register() function to use a
structure instead of list of arguments to describe new compression
protocol. That approach is more flexible and allows to extend the API in
the future without changing the already implemented protocols.

Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       | 18 ++++++++-----
 lib/trace-cmd/trace-compress-zlib.c           | 15 ++++++++---
 lib/trace-cmd/trace-compress-zstd.c           | 18 ++++++++-----
 lib/trace-cmd/trace-compress.c                | 26 +++++++------------
 4 files changed, 44 insertions(+), 33 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index d229b264..0ea37abc 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -514,6 +514,16 @@ struct tracecmd_compress_chunk {
 	off64_t			offset;
 };
 struct tracecmd_compression;
+struct tracecmd_compression_proto {
+	int weight;
+	const char *name;
+	const char *version;
+	int (*compress)(const char *in, unsigned int in_bytes, char *out, unsigned int *out_bytes);
+	int (*uncompress)(const char *in, unsigned int in_bytes, char *out, unsigned int *out_bytes);
+	unsigned int (*compress_size)(unsigned int bytes);
+	bool (*is_supported)(const char *name, const char *version);
+};
+
 struct tracecmd_compression *tracecmd_compress_alloc(const char *name, const char *version,
 						     int fd, struct tep_handle *tep,
 						     struct tracecmd_msg_handle *msg_handle);
@@ -530,13 +540,7 @@ int tracecmd_compress_proto_get_name(struct tracecmd_compression *compress,
 				     const char **name, const char **version);
 bool tracecmd_compress_is_supported(const char *name, const char *version);
 int tracecmd_compress_protos_get(char ***names, char ***versions);
-int tracecmd_compress_proto_register(const char *name, const char *version, int weight,
-				     int (*compress)(const char *in, unsigned int in_bytes,
-						     char *out, unsigned int *out_bytes),
-				     int (*uncompress)(const char *in, unsigned int in_bytes,
-						       char *out, unsigned int *out_bytes),
-				     unsigned int (*comress_size)(unsigned int bytes),
-				     bool (*is_supported)(const char *name, const char *version));
+int tracecmd_compress_proto_register(struct tracecmd_compression_proto *proto);
 int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int chunk_size,
 				unsigned long long *read_size, unsigned long long *write_size);
 int tracecmd_uncompress_copy_to(struct tracecmd_compression *handle, int fd,
diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c
index a697cc61..8b9758c9 100644
--- a/lib/trace-cmd/trace-compress-zlib.c
+++ b/lib/trace-cmd/trace-compress-zlib.c
@@ -103,7 +103,16 @@ static bool zlib_is_supported(const char *name, const char *version)
 
 int tracecmd_zlib_init(void)
 {
-	return tracecmd_compress_proto_register(__ZLIB_NAME, zlibVersion(), __ZLIB_WEIGTH,
-						zlib_compress, zlib_decompress,
-						zlib_compress_bound, zlib_is_supported);
+	struct tracecmd_compression_proto proto;
+
+	memset(&proto, 0, sizeof(proto));
+	proto.name = __ZLIB_NAME;
+	proto.version = zlibVersion();
+	proto.weight = __ZLIB_WEIGTH;
+	proto.compress = zlib_compress;
+	proto.uncompress = zlib_decompress;
+	proto.is_supported = zlib_is_supported;
+	proto.compress_size = zlib_compress_bound;
+
+	return tracecmd_compress_proto_register(&proto);
 }
diff --git a/lib/trace-cmd/trace-compress-zstd.c b/lib/trace-cmd/trace-compress-zstd.c
index fc5e350f..f99ad312 100644
--- a/lib/trace-cmd/trace-compress-zstd.c
+++ b/lib/trace-cmd/trace-compress-zstd.c
@@ -59,9 +59,19 @@ static bool zstd_is_supported(const char *name, const char *version)
 
 int tracecmd_zstd_init(void)
 {
+	struct tracecmd_compression_proto proto;
 	int ret = 0;
 	size_t r;
 
+	memset(&proto, 0, sizeof(proto));
+	proto.name = __ZSTD_NAME;
+	proto.version = ZSTD_versionString();
+	proto.weight = __ZSTD_WEIGTH;
+	proto.compress = zstd_compress;
+	proto.uncompress = zstd_decompress;
+	proto.is_supported = zstd_is_supported;
+	proto.compress_size = zstd_compress_bound;
+
 	ctx_c = ZSTD_createCCtx();
 	ctx_d = ZSTD_createDCtx();
 	if (!ctx_c || !ctx_d)
@@ -71,13 +81,7 @@ int tracecmd_zstd_init(void)
 	if (ZSTD_isError(r))
 		goto err;
 
-	ret = tracecmd_compress_proto_register(__ZSTD_NAME,
-					       ZSTD_versionString(),
-					       __ZSTD_WEIGTH,
-					       zstd_compress,
-					       zstd_decompress,
-					       zstd_compress_bound,
-					       zstd_is_supported);
+	ret = tracecmd_compress_proto_register(&proto);
 	if (!ret)
 		return 0;
 err:
diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
index 4fca7019..66bfc356 100644
--- a/lib/trace-cmd/trace-compress.c
+++ b/lib/trace-cmd/trace-compress.c
@@ -522,39 +522,33 @@ int tracecmd_compress_proto_get_name(struct tracecmd_compression *compress,
  * Returns 0 on success, or -1 in case of an error. If algorithm with given name
  * and version is already registered, -1 is returned.
  */
-int tracecmd_compress_proto_register(const char *name, const char *version, int weight,
-				     int (*compress)(const char *in, unsigned int in_bytes,
-						     char *out, unsigned int *out_bytes),
-				     int (*uncompress)(const char *in, unsigned int in_bytes,
-						       char *out, unsigned int *out_bytes),
-				     unsigned int (*compress_size)(unsigned int bytes),
-				     bool (*is_supported)(const char *name, const char *version))
+int tracecmd_compress_proto_register(struct tracecmd_compression_proto *proto)
 {
 	struct compress_proto *new;
 
-	if (!name || !compress || !uncompress)
+	if (!proto || !proto->name || !proto->compress || !proto->uncompress)
 		return -1;
 
-	if (tracecmd_compress_is_supported(name, version))
+	if (tracecmd_compress_is_supported(proto->name, proto->version))
 		return -1;
 
 	new = calloc(1, sizeof(*new));
 	if (!new)
 		return -1;
 
-	new->proto_name = strdup(name);
+	new->proto_name = strdup(proto->name);
 	if (!new->proto_name)
 		goto error;
 
-	new->proto_version = strdup(version);
+	new->proto_version = strdup(proto->version);
 	if (!new->proto_version)
 		goto error;
 
-	new->compress_block = compress;
-	new->uncompress_block = uncompress;
-	new->compress_size = compress_size;
-	new->is_supported = is_supported;
-	new->weight = weight;
+	new->compress_block = proto->compress;
+	new->uncompress_block = proto->uncompress;
+	new->compress_size = proto->compress_size;
+	new->is_supported = proto->is_supported;
+	new->weight = proto->weight;
 	new->next = proto_list;
 	proto_list = new;
 	return 0;
-- 
2.34.1

Re: [PATCH 1/5] trace-cmd: Use a structure to describe a compression protocol

[Sebastian Andrzej Siewior]

On 2022-03-02 06:51:27 [+0200], Tzvetomir Stoyanov (VMware) wrote:
> Changed the tracecmd_compress_proto_register() function to use a
> structure instead of list of arguments to describe new compression
> protocol. That approach is more flexible and allows to extend the API in
> the future without changing the already implemented protocols.
> 
> Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>

Thank you.

Acked-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

> index a697cc61..8b9758c9 100644
> --- a/lib/trace-cmd/trace-compress-zlib.c
> +++ b/lib/trace-cmd/trace-compress-zlib.c
> @@ -103,7 +103,16 @@ static bool zlib_is_supported(const char *name, const char *version)
>  
>  int tracecmd_zlib_init(void)
>  {
> -	return tracecmd_compress_proto_register(__ZLIB_NAME, zlibVersion(), __ZLIB_WEIGTH,
> -						zlib_compress, zlib_decompress,
> -						zlib_compress_bound, zlib_is_supported);
> +	struct tracecmd_compression_proto proto;
> +
> +	memset(&proto, 0, sizeof(proto));
> +	proto.name = __ZLIB_NAME;
> +	proto.version = zlibVersion();
> +	proto.weight = __ZLIB_WEIGTH;
> +	proto.compress = zlib_compress;
> +	proto.uncompress = zlib_decompress;
> +	proto.is_supported = zlib_is_supported;
> +	proto.compress_size = zlib_compress_bound;

	struct tracecmd_compression_proto proto = {
		.name = __ZLIB_NAME,
		.version = zlibVersion(),
		.weight = __ZLIB_WEIGTH,
		.compress = zlib_compress,
		.uncompress = zlib_decompress,
		.is_supported = zlib_is_supported,
		.compress_size = zlib_compress_bound,
	}

this way to initialize ensures that members which are not explicitly
touched are set to 0. And you don't have to type proto that often ;)

> +
> +	return tracecmd_compress_proto_register(&proto);
>  }

Sebastian

[PATCH 2/5] trace-cmd: Make internal compression hooks more generic

[Tzvetomir Stoyanov (VMware)]

Changed the prototypes of trace-cmd internal compression API to be more
generic.

Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  4 +--
 lib/trace-cmd/trace-compress-zlib.c           | 20 +++++------
 lib/trace-cmd/trace-compress-zstd.c           | 18 +++++-----
 lib/trace-cmd/trace-compress.c                | 33 ++++++++-----------
 4 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 0ea37abc..45d89270 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -518,8 +518,8 @@ struct tracecmd_compression_proto {
 	int weight;
 	const char *name;
 	const char *version;
-	int (*compress)(const char *in, unsigned int in_bytes, char *out, unsigned int *out_bytes);
-	int (*uncompress)(const char *in, unsigned int in_bytes, char *out, unsigned int *out_bytes);
+	int (*compress)(const void *in, int in_bytes, void *out, int out_bytes);
+	int (*uncompress)(const void *in, int in_bytes, void *out, int out_bytes);
 	unsigned int (*compress_size)(unsigned int bytes);
 	bool (*is_supported)(const char *name, const char *version);
 };
diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c
index 8b9758c9..41342597 100644
--- a/lib/trace-cmd/trace-compress-zlib.c
+++ b/lib/trace-cmd/trace-compress-zlib.c
@@ -13,19 +13,17 @@
 #define __ZLIB_NAME		"zlib"
 #define __ZLIB_WEIGTH		10
 
-static int zlib_compress(const char *in, unsigned int in_bytes,
-			 char *out, unsigned int *out_bytes)
+static int zlib_compress(const void *in, int in_bytes, void *out, int out_bytes)
 {
-	unsigned long out_size = *out_bytes;
+	unsigned long obytes = out_bytes;
 	int ret;
 
-	ret = compress2((unsigned char *)out, &out_size,
+	ret = compress2((unsigned char *)out, &obytes,
 			(unsigned char *)in, (unsigned long)in_bytes, Z_BEST_COMPRESSION);
-	*out_bytes = out_size;
 	errno = 0;
 	switch (ret) {
 	case Z_OK:
-		return 0;
+		return obytes;
 	case Z_BUF_ERROR:
 		errno = -ENOBUFS;
 		break;
@@ -43,19 +41,17 @@ static int zlib_compress(const char *in, unsigned int in_bytes,
 	return -1;
 }
 
-static int zlib_decompress(const char *in, unsigned int in_bytes,
-			   char *out, unsigned int *out_bytes)
+static int zlib_decompress(const void *in, int in_bytes, void *out, int out_bytes)
 {
-	unsigned long out_size = *out_bytes;
+	unsigned long obytes = out_bytes;
 	int ret;
 
-	ret = uncompress((unsigned char *)out, &out_size,
+	ret = uncompress((unsigned char *)out, &obytes,
 			 (unsigned char *)in, (unsigned long)in_bytes);
-	*out_bytes = out_size;
 	errno = 0;
 	switch (ret) {
 	case Z_OK:
-		return 0;
+		return obytes;
 	case Z_BUF_ERROR:
 		errno = -ENOBUFS;
 		break;
diff --git a/lib/trace-cmd/trace-compress-zstd.c b/lib/trace-cmd/trace-compress-zstd.c
index f99ad312..eee4e5d6 100644
--- a/lib/trace-cmd/trace-compress-zstd.c
+++ b/lib/trace-cmd/trace-compress-zstd.c
@@ -15,31 +15,29 @@
 static ZSTD_CCtx *ctx_c;
 static ZSTD_DCtx *ctx_d;
 
-static int zstd_compress(const char *in, unsigned int in_bytes,
-			 char *out, unsigned int *out_bytes)
+static int zstd_compress(const void *in, int in_bytes, void *out, int out_bytes)
 {
 	size_t ret;
 
-	ret = ZSTD_compress2(ctx_c, out, *out_bytes, in, in_bytes);
+	ret = ZSTD_compress2(ctx_c, out, out_bytes, in, in_bytes);
 	if (ZSTD_isError(ret))
 		return -1;
-	*out_bytes = ret;
-	return 0;
+
+	return ret;
 }
 
-static int zstd_decompress(const char *in, unsigned int in_bytes,
-			   char *out, unsigned int *out_bytes)
+static int zstd_decompress(const void *in, int in_bytes, void *out, int out_bytes)
 {
 	size_t ret;
 
-	ret = ZSTD_decompressDCtx(ctx_d, out, *out_bytes, in, in_bytes);
+	ret = ZSTD_decompressDCtx(ctx_d, out, out_bytes, in, in_bytes);
 	if (ZSTD_isError(ret)) {
 		errno = -EINVAL;
 		return -1;
 	}
-	*out_bytes = ret;
+
 	errno = 0;
-	return 0;
+	return ret;
 }
 
 static unsigned int zstd_compress_bound(unsigned int in_bytes)
diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
index 66bfc356..6263439c 100644
--- a/lib/trace-cmd/trace-compress.c
+++ b/lib/trace-cmd/trace-compress.c
@@ -18,10 +18,8 @@ struct compress_proto {
 	char *proto_version;
 	int weight;
 
-	int (*compress_block)(const char *in, unsigned int in_bytes,
-			     char *out, unsigned int *out_bytes);
-	int (*uncompress_block)(const char *in, unsigned int in_bytes,
-				char *out, unsigned int *out_bytes);
+	int (*compress_block)(const void *in, int in_bytes, void *out, int out_bytes);
+	int (*uncompress_block)(const void *in,  int in_bytes, void *out, int out_bytes);
 	unsigned int (*compress_size)(unsigned int bytes);
 	bool (*is_supported)(const char *name, const char *version);
 };
@@ -273,15 +271,13 @@ int tracecmd_uncompress_block(struct tracecmd_compression *handle)
 	if (read_fd(handle->fd, bytes, s_compressed) < 0)
 		goto error;
 
-	s_uncompressed = size;
-	ret = handle->proto->uncompress_block(bytes, s_compressed,
-					      handle->buffer, &s_uncompressed);
-	if (ret)
+	ret = handle->proto->uncompress_block(bytes, s_compressed, handle->buffer, size);
+	if (ret < 0)
 		goto error;
 
 	free(bytes);
 	handle->pointer = 0;
-	handle->capacity_read = s_uncompressed;
+	handle->capacity_read = ret;
 	handle->capacity = size;
 	return 0;
 error:
@@ -318,12 +314,12 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
 	if (!buf)
 		return -1;
 
-	ret = handle->proto->compress_block(handle->buffer, handle->pointer, buf, &size);
+	ret = handle->proto->compress_block(handle->buffer, handle->pointer, buf, size);
 	if (ret < 0)
 		goto out;
 
 	/* Write compressed data size */
-	endian4 = tep_read_number(handle->tep, &size, 4);
+	endian4 = tep_read_number(handle->tep, &ret, 4);
 	ret = do_write(handle, &endian4, 4);
 	if (ret != 4)
 		goto out;
@@ -710,12 +706,13 @@ int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int
 		rsize += all;
 		size = csize;
 		if (all > 0) {
-			ret = handle->proto->compress_block(buf_from, all, buf_to, &size);
+			ret = handle->proto->compress_block(buf_from, all, buf_to, size);
 			if (ret < 0) {
 				if (errno == EINTR)
 					continue;
 				break;
 			}
+			size = ret;
 			/* Write compressed data size */
 			endian4 = tep_read_number(handle->tep, &size, 4);
 			ret = write_fd(handle->fd, &endian4, 4);
@@ -851,7 +848,6 @@ int tracecmd_uncompress_chunk(struct tracecmd_compression *handle,
 			      struct tracecmd_compress_chunk *chunk, char *data)
 {
 	char *bytes_in = NULL;
-	unsigned int size;
 	int ret = -1;
 
 	if (!handle || !handle->proto || !handle->proto->uncompress_block || !chunk || !data)
@@ -867,8 +863,7 @@ int tracecmd_uncompress_chunk(struct tracecmd_compression *handle,
 	if (read_fd(handle->fd, bytes_in, chunk->zsize) < 0)
 		goto out;
 
-	size = chunk->size;
-	if (handle->proto->uncompress_block(bytes_in, chunk->zsize, data, &size))
+	if (handle->proto->uncompress_block(bytes_in, chunk->zsize, data, chunk->size) < 0)
 		goto out;
 
 	ret = 0;
@@ -954,12 +949,12 @@ int tracecmd_uncompress_copy_to(struct tracecmd_compression *handle, int fd,
 
 		rsize += s_compressed;
 		ret = handle->proto->uncompress_block(bytes_in, s_compressed,
-						      bytes_out, &s_uncompressed);
-		if (ret)
+						      bytes_out, s_uncompressed);
+		if (ret < 0)
 			break;
 
-		write_fd(fd, bytes_out, s_uncompressed);
-		wsize += s_uncompressed;
+		write_fd(fd, bytes_out, ret);
+		wsize += ret;
 		chunks--;
 	}
 	free(bytes_in);
-- 
2.34.1

Re: [PATCH 2/5] trace-cmd: Make internal compression hooks more generic

[Sebastian Andrzej Siewior]

On 2022-03-02 06:51:28 [+0200], Tzvetomir Stoyanov (VMware) wrote:
> Changed the prototypes of trace-cmd internal compression API to be more
> generic.
> 
> Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>

Acked-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

> diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c
> index 8b9758c9..41342597 100644
> --- a/lib/trace-cmd/trace-compress-zlib.c
> +++ b/lib/trace-cmd/trace-compress-zlib.c
> @@ -13,19 +13,17 @@
>  #define __ZLIB_NAME		"zlib"
>  #define __ZLIB_WEIGTH		10
>  
> -static int zlib_compress(const char *in, unsigned int in_bytes,
> -			 char *out, unsigned int *out_bytes)
> +static int zlib_compress(const void *in, int in_bytes, void *out, int out_bytes)
>  {
> -	unsigned long out_size = *out_bytes;
> +	unsigned long obytes = out_bytes;
>  	int ret;
>  
> -	ret = compress2((unsigned char *)out, &out_size,
> +	ret = compress2((unsigned char *)out, &obytes,
>  			(unsigned char *)in, (unsigned long)in_bytes, Z_BEST_COMPRESSION);
there is no need to cast to or from void *, this works always.

> -	*out_bytes = out_size;
>  	errno = 0;
>  	switch (ret) {
>  	case Z_OK:
> -		return 0;
> +		return obytes;
>  	case Z_BUF_ERROR:
>  		errno = -ENOBUFS;
>  		break;

[PATCH 3/5] trace-cmd: Use errno from zlib, if available

[Tzvetomir Stoyanov (VMware)]

Some zlib APIs set the errno in case of an error and return Z_ERRNO.
In these cases, errno should not be overwritten by the tarce-cmd zlib
wrappers.

Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-compress-zlib.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c
index 41342597..0dfd4f15 100644
--- a/lib/trace-cmd/trace-compress-zlib.c
+++ b/lib/trace-cmd/trace-compress-zlib.c
@@ -33,6 +33,8 @@ static int zlib_compress(const void *in, int in_bytes, void *out, int out_bytes)
 	case Z_STREAM_ERROR:
 		errno = -EINVAL;
 		break;
+	case Z_ERRNO:
+		break;
 	default:
 		errno = -EFAULT;
 		break;
@@ -61,6 +63,8 @@ static int zlib_decompress(const void *in, int in_bytes, void *out, int out_byte
 	case Z_DATA_ERROR:
 		errno = -EINVAL;
 		break;
+	case Z_ERRNO:
+		break;
 	default:
 		errno = -EFAULT;
 		break;
-- 
2.34.1

Re: [PATCH 3/5] trace-cmd: Use errno from zlib, if available

[Sebastian Andrzej Siewior]

On 2022-03-02 06:51:29 [+0200], Tzvetomir Stoyanov (VMware) wrote:
> Some zlib APIs set the errno in case of an error and return Z_ERRNO.
> In these cases, errno should not be overwritten by the tarce-cmd zlib
> wrappers.
> 
> Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  lib/trace-cmd/trace-compress-zlib.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c
> index 41342597..0dfd4f15 100644
> --- a/lib/trace-cmd/trace-compress-zlib.c
> +++ b/lib/trace-cmd/trace-compress-zlib.c
> @@ -33,6 +33,8 @@ static int zlib_compress(const void *in, int in_bytes, void *out, int out_bytes)
>  	case Z_STREAM_ERROR:
>  		errno = -EINVAL;
>  		break;
> +	case Z_ERRNO:
> +		break;
>  	default:
>  		errno = -EFAULT;
>  		break;
> @@ -61,6 +63,8 @@ static int zlib_decompress(const void *in, int in_bytes, void *out, int out_byte
>  	case Z_DATA_ERROR:
>  		errno = -EINVAL;
>  		break;
> +	case Z_ERRNO:
> +		break;
>  	default:
>  		errno = -EFAULT;
>  		break;

I was thinking about returning the error for compress/decompress via the
return value and not touching errno at all.

Sebastian

Re: [PATCH 3/5] trace-cmd: Use errno from zlib, if available

[Steven Rostedt]

On Wed, 2 Mar 2022 08:15:30 +0100
Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:

> I was thinking about returning the error for compress/decompress via the
> return value and not touching errno at all.

I know you hate the use of ERRNO in libraries and tooling, but we've decided
to go that way for all of trace-cmd and the libraries (libtraceevent and
libtracefs). This is just being consistent with all the other callers, and
to do it differently will create an inconsistency in the API.

-- Steve

Re: [PATCH 3/5] trace-cmd: Use errno from zlib, if available

[Sebastian Andrzej Siewior]

On 2022-03-02 10:52:30 [-0500], Steven Rostedt wrote:
> On Wed, 2 Mar 2022 08:15:30 +0100
> Sebastian Andrzej Siewior <sebastian@breakpoint.cc> wrote:
> 
> > I was thinking about returning the error for compress/decompress via the
> > return value and not touching errno at all.
> 
> I know you hate the use of ERRNO in libraries and tooling, but we've decided
> to go that way for all of trace-cmd and the libraries (libtraceevent and
> libtracefs). This is just being consistent with all the other callers, and
> to do it differently will create an inconsistency in the API.

Ah okay then. In that case, the zstd interface does not set errno in the
compress or decompress callback. This might need an update to be
consistent. Otherwise the return value is < 0 and errno might still be
0.

> -- Steve

Sebastian

[PATCH 4/5] trace-cmd: Add context to compression hooks

[Tzvetomir Stoyanov (VMware)]

Some compression libraries require a context for compression and
decompression. Currently the internal compression API does not have such
context. That could cause problems in the future, if a multithread
compression logic is implemented.

Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 .../include/private/trace-cmd-private.h       |  8 +++--
 lib/trace-cmd/trace-compress-zlib.c           |  6 ++--
 lib/trace-cmd/trace-compress-zstd.c           |  6 ++--
 lib/trace-cmd/trace-compress.c                | 33 +++++++++++++------
 4 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/lib/trace-cmd/include/private/trace-cmd-private.h b/lib/trace-cmd/include/private/trace-cmd-private.h
index 45d89270..69343765 100644
--- a/lib/trace-cmd/include/private/trace-cmd-private.h
+++ b/lib/trace-cmd/include/private/trace-cmd-private.h
@@ -518,10 +518,12 @@ struct tracecmd_compression_proto {
 	int weight;
 	const char *name;
 	const char *version;
-	int (*compress)(const void *in, int in_bytes, void *out, int out_bytes);
-	int (*uncompress)(const void *in, int in_bytes, void *out, int out_bytes);
-	unsigned int (*compress_size)(unsigned int bytes);
+	int (*compress)(void *ctx, const void *in, int in_bytes, void *out, int out_bytes);
+	int (*uncompress)(void *ctx, const void *in, int in_bytes, void *out, int out_bytes);
+	unsigned int (*compress_size)(void *ctx, unsigned int bytes);
 	bool (*is_supported)(const char *name, const char *version);
+	void *(*new_context)(void);
+	void (*free_context)(void *ctx);
 };
 
 struct tracecmd_compression *tracecmd_compress_alloc(const char *name, const char *version,
diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c
index 0dfd4f15..819a739d 100644
--- a/lib/trace-cmd/trace-compress-zlib.c
+++ b/lib/trace-cmd/trace-compress-zlib.c
@@ -13,7 +13,7 @@
 #define __ZLIB_NAME		"zlib"
 #define __ZLIB_WEIGTH		10
 
-static int zlib_compress(const void *in, int in_bytes, void *out, int out_bytes)
+static int zlib_compress(void *ctx, const void *in, int in_bytes, void *out, int out_bytes)
 {
 	unsigned long obytes = out_bytes;
 	int ret;
@@ -43,7 +43,7 @@ static int zlib_compress(const void *in, int in_bytes, void *out, int out_bytes)
 	return -1;
 }
 
-static int zlib_decompress(const void *in, int in_bytes, void *out, int out_bytes)
+static int zlib_decompress(void *ctx, const void *in, int in_bytes, void *out, int out_bytes)
 {
 	unsigned long obytes = out_bytes;
 	int ret;
@@ -73,7 +73,7 @@ static int zlib_decompress(const void *in, int in_bytes, void *out, int out_byte
 	return -1;
 }
 
-static unsigned int zlib_compress_bound(unsigned int in_bytes)
+static unsigned int zlib_compress_bound(void *ctx, unsigned int in_bytes)
 {
 	return compressBound(in_bytes);
 }
diff --git a/lib/trace-cmd/trace-compress-zstd.c b/lib/trace-cmd/trace-compress-zstd.c
index eee4e5d6..98eaac00 100644
--- a/lib/trace-cmd/trace-compress-zstd.c
+++ b/lib/trace-cmd/trace-compress-zstd.c
@@ -15,7 +15,7 @@
 static ZSTD_CCtx *ctx_c;
 static ZSTD_DCtx *ctx_d;
 
-static int zstd_compress(const void *in, int in_bytes, void *out, int out_bytes)
+static int zstd_compress(void *ctx, const void *in, int in_bytes, void *out, int out_bytes)
 {
 	size_t ret;
 
@@ -26,7 +26,7 @@ static int zstd_compress(const void *in, int in_bytes, void *out, int out_bytes)
 	return ret;
 }
 
-static int zstd_decompress(const void *in, int in_bytes, void *out, int out_bytes)
+static int zstd_decompress(void *ctx, const void *in, int in_bytes, void *out, int out_bytes)
 {
 	size_t ret;
 
@@ -40,7 +40,7 @@ static int zstd_decompress(const void *in, int in_bytes, void *out, int out_byte
 	return ret;
 }
 
-static unsigned int zstd_compress_bound(unsigned int in_bytes)
+static unsigned int zstd_compress_bound(void *ctx, unsigned int in_bytes)
 {
 	return ZSTD_compressBound(in_bytes);
 }
diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
index 6263439c..9371d3cc 100644
--- a/lib/trace-cmd/trace-compress.c
+++ b/lib/trace-cmd/trace-compress.c
@@ -18,10 +18,12 @@ struct compress_proto {
 	char *proto_version;
 	int weight;
 
-	int (*compress_block)(const void *in, int in_bytes, void *out, int out_bytes);
-	int (*uncompress_block)(const void *in,  int in_bytes, void *out, int out_bytes);
-	unsigned int (*compress_size)(unsigned int bytes);
+	int (*compress_block)(void *ctx, const void *in, int in_bytes, void *out, int out_bytes);
+	int (*uncompress_block)(void *ctx, const void *in,  int in_bytes, void *out, int out_bytes);
+	unsigned int (*compress_size)(void *ctx, unsigned int bytes);
 	bool (*is_supported)(const char *name, const char *version);
+	void *(*new_context)(void);
+	void (*free_context)(void *ctx);
 };
 
 static struct compress_proto *proto_list;
@@ -35,6 +37,7 @@ struct tracecmd_compression {
 	struct compress_proto		*proto;
 	struct tep_handle		*tep;
 	struct tracecmd_msg_handle	*msg_handle;
+	void				*context;
 };
 
 static int read_fd(int fd, char *dst, int len)
@@ -271,7 +274,8 @@ int tracecmd_uncompress_block(struct tracecmd_compression *handle)
 	if (read_fd(handle->fd, bytes, s_compressed) < 0)
 		goto error;
 
-	ret = handle->proto->uncompress_block(bytes, s_compressed, handle->buffer, size);
+	ret = handle->proto->uncompress_block(handle->context,
+					      bytes, s_compressed, handle->buffer, size);
 	if (ret < 0)
 		goto error;
 
@@ -308,13 +312,13 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
 	    !handle->proto->compress_size || !handle->proto->compress_block)
 		return -1;
 
-	size = handle->proto->compress_size(handle->pointer);
+	size = handle->proto->compress_size(handle->context, handle->pointer);
 
 	buf = malloc(size);
 	if (!buf)
 		return -1;
 
-	ret = handle->proto->compress_block(handle->buffer, handle->pointer, buf, size);
+	ret = handle->proto->compress_block(handle->context, handle->buffer, handle->pointer, buf, size);
 	if (ret < 0)
 		goto out;
 
@@ -443,6 +447,9 @@ struct tracecmd_compression *tracecmd_compress_alloc(const char *name, const cha
 	new->tep = tep;
 	new->msg_handle = msg_handle;
 	new->proto = proto;
+	if (proto->new_context)
+		new->context = proto->new_context();
+
 	return new;
 }
 
@@ -453,6 +460,8 @@ struct tracecmd_compression *tracecmd_compress_alloc(const char *name, const cha
 void tracecmd_compress_destroy(struct tracecmd_compression *handle)
 {
 	tracecmd_compress_reset(handle);
+	if (handle->proto->free_context)
+		handle->proto->free_context(handle->context);
 	free(handle);
 }
 
@@ -546,6 +555,8 @@ int tracecmd_compress_proto_register(struct tracecmd_compression_proto *proto)
 	new->is_supported = proto->is_supported;
 	new->weight = proto->weight;
 	new->next = proto_list;
+	new->new_context = proto->new_context;
+	new->free_context = proto->free_context;
 	proto_list = new;
 	return 0;
 
@@ -672,7 +683,7 @@ int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int
 
 	if (read_size)
 		rmax = *read_size;
-	csize = handle->proto->compress_size(chunk_size);
+	csize = handle->proto->compress_size(handle->context, chunk_size);
 	buf_from = malloc(chunk_size);
 	if (!buf_from)
 		return -1;
@@ -706,7 +717,8 @@ int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int
 		rsize += all;
 		size = csize;
 		if (all > 0) {
-			ret = handle->proto->compress_block(buf_from, all, buf_to, size);
+			ret = handle->proto->compress_block(handle->context,
+							    buf_from, all, buf_to, size);
 			if (ret < 0) {
 				if (errno == EINTR)
 					continue;
@@ -863,7 +875,8 @@ int tracecmd_uncompress_chunk(struct tracecmd_compression *handle,
 	if (read_fd(handle->fd, bytes_in, chunk->zsize) < 0)
 		goto out;
 
-	if (handle->proto->uncompress_block(bytes_in, chunk->zsize, data, chunk->size) < 0)
+	if (handle->proto->uncompress_block(handle->context,
+					    bytes_in, chunk->zsize, data, chunk->size) < 0)
 		goto out;
 
 	ret = 0;
@@ -948,7 +961,7 @@ int tracecmd_uncompress_copy_to(struct tracecmd_compression *handle, int fd,
 			break;
 
 		rsize += s_compressed;
-		ret = handle->proto->uncompress_block(bytes_in, s_compressed,
+		ret = handle->proto->uncompress_block(handle->context, bytes_in, s_compressed,
 						      bytes_out, s_uncompressed);
 		if (ret < 0)
 			break;
-- 
2.34.1

Re: [PATCH 4/5] trace-cmd: Add context to compression hooks

[Sebastian Andrzej Siewior]

On 2022-03-02 06:51:30 [+0200], Tzvetomir Stoyanov (VMware) wrote:
> Some compression libraries require a context for compression and
> decompression. Currently the internal compression API does not have such
> context. That could cause problems in the future, if a multithread
> compression logic is implemented.
> 
> Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>

Acked-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Sebastian

Re: [PATCH 4/5] trace-cmd: Add context to compression hooks

[Steven Rostedt]

On Wed,  2 Mar 2022 06:51:30 +0200
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> @@ -453,6 +460,8 @@ struct tracecmd_compression *tracecmd_compress_alloc(const char *name, const cha
>  void tracecmd_compress_destroy(struct tracecmd_compression *handle)
>  {
>  	tracecmd_compress_reset(handle);
> +	if (handle->proto->free_context)
> +		handle->proto->free_context(handle->context);
>  	free(handle);
>  }
>  

I segfaulted here with:

	# trace-cmd restore -c -o /tmp/head

Need to test handle is NULL or not.

Perhaps even just exit normally at the start?

	if (!handle)
		return;

Which makes it not crash.

-- Steve

Re: [PATCH 4/5] trace-cmd: Add context to compression hooks

[Tzvetomir Stoyanov]

On Thu, Mar 3, 2022 at 3:10 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed,  2 Mar 2022 06:51:30 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > @@ -453,6 +460,8 @@ struct tracecmd_compression *tracecmd_compress_alloc(const char *name, const cha
> >  void tracecmd_compress_destroy(struct tracecmd_compression *handle)
> >  {
> >       tracecmd_compress_reset(handle);
> > +     if (handle->proto->free_context)
> > +             handle->proto->free_context(handle->context);
> >       free(handle);
> >  }
> >
>
> I segfaulted here with:
>
>         # trace-cmd restore -c -o /tmp/head
>
> Need to test handle is NULL or not.
>
> Perhaps even just exit normally at the start?
>
>         if (!handle)
>                 return;
>

ops :)  I'll send v2 of the patch set with this fix.

Thanks Steven!

> Which makes it not crash.
>
> -- Steve



-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

[PATCH 5/5] trace-cmd: Use context hooks in zstd

[Tzvetomir Stoyanov (VMware)]

Using global compression context could be a limitation in multithreaded
use cases. Use context hooks instead of global compression context.

Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
---
 lib/trace-cmd/trace-compress-zstd.c | 80 +++++++++++++++++++----------
 1 file changed, 54 insertions(+), 26 deletions(-)

diff --git a/lib/trace-cmd/trace-compress-zstd.c b/lib/trace-cmd/trace-compress-zstd.c
index 98eaac00..3e42fc79 100644
--- a/lib/trace-cmd/trace-compress-zstd.c
+++ b/lib/trace-cmd/trace-compress-zstd.c
@@ -12,14 +12,20 @@
 #define __ZSTD_NAME		"zstd"
 #define __ZSTD_WEIGTH		5
 
-static ZSTD_CCtx *ctx_c;
-static ZSTD_DCtx *ctx_d;
+struct zstd_context {
+	ZSTD_CCtx *ctx_c;
+	ZSTD_DCtx *ctx_d;
+};
 
 static int zstd_compress(void *ctx, const void *in, int in_bytes, void *out, int out_bytes)
 {
+	struct zstd_context *context = ctx;
 	size_t ret;
 
-	ret = ZSTD_compress2(ctx_c, out, out_bytes, in, in_bytes);
+	if (!ctx)
+		return -1;
+
+	ret = ZSTD_compress2(context->ctx_c, out, out_bytes, in, in_bytes);
 	if (ZSTD_isError(ret))
 		return -1;
 
@@ -28,9 +34,13 @@ static int zstd_compress(void *ctx, const void *in, int in_bytes, void *out, int
 
 static int zstd_decompress(void *ctx, const void *in, int in_bytes, void *out, int out_bytes)
 {
+	struct zstd_context *context = ctx;
 	size_t ret;
 
-	ret = ZSTD_decompressDCtx(ctx_d, out, out_bytes, in, in_bytes);
+	if (!ctx)
+		return -1;
+
+	ret = ZSTD_decompressDCtx(context->ctx_d, out, out_bytes, in, in_bytes);
 	if (ZSTD_isError(ret)) {
 		errno = -EINVAL;
 		return -1;
@@ -55,11 +65,46 @@ static bool zstd_is_supported(const char *name, const char *version)
 	return true;
 }
 
+static void *new_zstd_context(void)
+{
+	struct zstd_context *context;
+	size_t r;
+
+	context = calloc(1, sizeof(*context));
+	if (!context)
+		return NULL;
+
+	context->ctx_c = ZSTD_createCCtx();
+	context->ctx_d = ZSTD_createDCtx();
+	if (!context->ctx_c || !context->ctx_d)
+		goto err;
+
+	r = ZSTD_CCtx_setParameter(context->ctx_c, ZSTD_c_contentSizeFlag, 0);
+	if (ZSTD_isError(r))
+		goto err;
+
+	return context;
+err:
+	ZSTD_freeCCtx(context->ctx_c);
+	ZSTD_freeDCtx(context->ctx_d);
+	free(context);
+	return NULL;
+}
+static void free_zstd_context(void *ctx)
+{
+	struct zstd_context *context = ctx;
+
+	if (!ctx)
+		return;
+
+	ZSTD_freeCCtx(context->ctx_c);
+	ZSTD_freeDCtx(context->ctx_d);
+	free(context);
+}
+
 int tracecmd_zstd_init(void)
 {
 	struct tracecmd_compression_proto proto;
-	int ret = 0;
-	size_t r;
 
 	memset(&proto, 0, sizeof(proto));
 	proto.name = __ZSTD_NAME;
@@ -69,25 +114,8 @@ int tracecmd_zstd_init(void)
 	proto.uncompress = zstd_decompress;
 	proto.is_supported = zstd_is_supported;
 	proto.compress_size = zstd_compress_bound;
+	proto.new_context = new_zstd_context;
+	proto.free_context = free_zstd_context;
 
-	ctx_c = ZSTD_createCCtx();
-	ctx_d = ZSTD_createDCtx();
-	if (!ctx_c || !ctx_d)
-		goto err;
-
-	r = ZSTD_CCtx_setParameter(ctx_c, ZSTD_c_contentSizeFlag, 0);
-	if (ZSTD_isError(r))
-		goto err;
-
-	ret = tracecmd_compress_proto_register(&proto);
-	if (!ret)
-		return 0;
-err:
-	ZSTD_freeCCtx(ctx_c);
-	ZSTD_freeDCtx(ctx_d);
-	ctx_c = NULL;
-	ctx_d = NULL;
-	if (ret < 0)
-		return ret;
-	return -1;
+	return tracecmd_compress_proto_register(&proto);
 }
-- 
2.34.1

Re: [PATCH 5/5] trace-cmd: Use context hooks in zstd

[Sebastian Andrzej Siewior]

On 2022-03-02 06:51:31 [+0200], Tzvetomir Stoyanov (VMware) wrote:
> Using global compression context could be a limitation in multithreaded
> use cases. Use context hooks instead of global compression context.
> 
> Suggested-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>

Acked-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>

Sebastian