| Message ID | 20210614075029.598048-18-tz.stoyanov@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add trace file compression | expand |
On Mon, 14 Jun 2021 10:50:01 +0300 "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > If libz is available, use that library to provide trace file compression > support. The library is detected runtime. Why have the library detected at runtime? If it is detected, we can then have the library flags include -lz. Why use dlopen to load zlib? And not just include it? This seems rather fragile to try to get right. -- Steve > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > ---
On Tue, Jun 22, 2021 at 4:26 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Mon, 14 Jun 2021 10:50:01 +0300 > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > > If libz is available, use that library to provide trace file compression > > support. The library is detected runtime. > > Why have the library detected at runtime? > > If it is detected, we can then have the library flags include -lz. > > Why use dlopen to load zlib? And not just include it? > > This seems rather fragile to try to get right. The idea of this design is not to bring additional mandatory dependencies. I do not know if libz is available by default, but even if we assume it is - each additional compression library that is added will be a mandatory dependency. The compression is not a mandatory functionality, trace-cmd can work without it. Why do you think it is fragile, I think dlopen() uses the same core linker logic to find and load the library. The only difference I see is that using "-lz" leads to a mandatory dependency, trace-cmd will not run without it. > > -- Steve > > > > > > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> > > --- -- Tzvetomir (Ceco) Stoyanov VMware Open Source Technology Center
On Tue, 22 Jun 2021 13:29:36 +0300 Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote: > On Tue, Jun 22, 2021 at 4:26 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > On Mon, 14 Jun 2021 10:50:01 +0300 > > "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote: > > > > > If libz is available, use that library to provide trace file compression > > > support. The library is detected runtime. > > > > Why have the library detected at runtime? > > > > If it is detected, we can then have the library flags include -lz. > > > > Why use dlopen to load zlib? And not just include it? > > > > This seems rather fragile to try to get right. > > The idea of this design is not to bring additional mandatory > dependencies. I do not know if libz is available by default, but even > if we assume it is - each additional compression library that is added > will be a mandatory dependency. The compression is not a mandatory > functionality, trace-cmd can work without it. Why do you think it is > fragile, I think dlopen() uses the same core linker logic to find and > load the library. The only difference I see is that using "-lz" leads > to a mandatory dependency, trace-cmd will not run without it. > The dependency is determined at compile time, just like we do for the python libraries, and dwarf, and anything else that might add a new feature. Let me explain a scenario of why I called it fragile. Let's say that trace-cmd has no package dependency on zlib, but the system has some other package that does have a dependency. Thus the package manager pulls in zlib to satisfy this other package. The user does a recording, and trace-cmd detects zlib, and compresses the data. Later, the user decides they do not need this other package and uninstalls it. The package manager sees there's nothing that depends on zlib anymore, and uninstalls zlib as well. Then the user goes to read their trace.dat file, and suddenly trace-cmd can't read it! THAT is what I call fragile. And if something like that ever happened to me, I would stop using whatever did that to me. Manually pulling in system dynamic libraries with dlopen is something I never heard of. The way to do this is make it a build time dependency. If zlib exists, then define HAVE_ZLIB and allow compressions and everything else. If it does not, then we don't support compression. Simple as that. Making it a runtime dependency has a lot of issues, *especially* since one run of trace-cmd (the reporting) depends on a previous run of trace-cmd (the recording), and if the environment changes between the two, the user will rightfully say WTF! -- Steve
diff --git a/lib/trace-cmd/Makefile b/lib/trace-cmd/Makefile index bab4322d..83ba7016 100644 --- a/lib/trace-cmd/Makefile +++ b/lib/trace-cmd/Makefile @@ -7,6 +7,13 @@ ldir:=$(src)/lib/trace-cmd DEFAULT_TARGET = $(LIBTRACECMD_STATIC) +pound := \# +ZLIB_INSTALLED := $(shell if (printf "$(pound)include <zlib.h>\n void main(){deflateInit(NULL, Z_BEST_COMPRESSION);}" | $(CC) -o /dev/null -x c - -lz >/dev/null 2>&1) ; then echo 1; else echo 0 ; fi) +ifeq ($(ZLIB_INSTALLED), 1) +CFLAGS += -DHAVE_ZLIB +$(info Have zlib compression support) +endif + OBJS = OBJS += trace-hash.o OBJS += trace-hooks.o @@ -26,6 +33,9 @@ OBJS += trace-timesync-ptp.o OBJS += trace-timesync-kvm.o endif OBJS += trace-compress.o +ifeq ($(ZLIB_INSTALLED), 1) +OBJS += trace-compress-zlib.o +endif # Additional util objects OBJS += trace-blk-hack.o diff --git a/lib/trace-cmd/include/trace-cmd-local.h b/lib/trace-cmd/include/trace-cmd-local.h index 419bfee0..40a6a40d 100644 --- a/lib/trace-cmd/include/trace-cmd-local.h +++ b/lib/trace-cmd/include/trace-cmd-local.h @@ -31,6 +31,11 @@ void tracecmd_info(const char *fmt, ...); #endif #endif +#ifdef HAVE_ZLIB +int tracecmd_zlib_init(void); +void tracecmd_zlib_free(void); +#endif + void tracecmd_compress_init(void); void tracecmd_compress_free(void); diff --git a/lib/trace-cmd/trace-compress-zlib.c b/lib/trace-cmd/trace-compress-zlib.c new file mode 100644 index 00000000..3208d57b --- /dev/null +++ b/lib/trace-cmd/trace-compress-zlib.c @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: LGPL-2.1 +/* + * Copyright (C) 2021, VMware, Tzvetomir Stoyanov tz.stoyanov@gmail.com> + * + */ +#include <stdlib.h> +#include <dlfcn.h> +#include <zlib.h> +#include <errno.h> + +#include "trace-cmd-private.h" + +#define __ZLIB_NAME "zlib" +#define __ZLIB_WEIGTH 10 +#define __ZLIB_FILE "libz.so" +#define ZLIB_FUNC_COMPRESS "compress2" +#define ZLIB_FUNC_DECOMOPRESS "uncompress" +#define ZLIB_FUNC_SIZE "compressBound" +#define ZLIB_FUNC_VERSION "zlibVersion" + +static void *zlib_handle; +static int (*_lib_compress)(unsigned char *out, unsigned long *out_bytes, + unsigned char *in, unsigned long in_bytes, int level); +static int (*_libz_decompress)(unsigned char *out, unsigned long *out_bytes, + unsigned char *in, unsigned long in_bytes); +static unsigned long (*_libz_compress_bound)(unsigned long in_bytes); +static const char *(*_libz_version)(void); + +static int zlib_compress(char *in, unsigned int in_bytes, + char *out, unsigned int *out_bytes) +{ + unsigned long out_size = *out_bytes; + int ret; + + if (!_lib_compress) + return -1; + + ret = _lib_compress((unsigned char *)out, &out_size, + (unsigned char *)in, (unsigned long)in_bytes, Z_BEST_COMPRESSION); + *out_bytes = out_size; + errno = 0; + switch (ret) { + case Z_OK: + return 0; + case Z_BUF_ERROR: + errno = -ENOBUFS; + break; + case Z_MEM_ERROR: + errno = -ENOMEM; + break; + case Z_STREAM_ERROR: + errno = -EINVAL; + break; + default: + errno = -EFAULT; + break; + } + + return -1; +} + +static int zlib_decompress(char *in, unsigned int in_bytes, + char *out, unsigned int *out_bytes) +{ + unsigned long out_size = *out_bytes; + int ret; + + if (!_libz_decompress) + return -1; + + ret = _libz_decompress((unsigned char *)out, &out_size, + (unsigned char *)in, (unsigned long)in_bytes); + *out_bytes = out_size; + errno = 0; + switch (ret) { + case Z_OK: + return 0; + case Z_BUF_ERROR: + errno = -ENOBUFS; + break; + case Z_MEM_ERROR: + errno = -ENOMEM; + break; + case Z_DATA_ERROR: + errno = -EINVAL; + break; + default: + errno = -EFAULT; + break; + } + + return -1; +} + +static unsigned int zlib_compress_bound(unsigned int in_bytes) +{ + if (!_libz_compress_bound) + return 0; + return _libz_compress_bound(in_bytes); +} + +static bool zlib_is_supported(const char *name, const char *version) +{ + const char *zver; + + if (!name) + return false; + if (strlen(name) != strlen(__ZLIB_NAME) || strcmp(name, __ZLIB_NAME)) + return false; + + if (!version) + return true; + + if (!_libz_version) + return false; + zver = _libz_version(); + if (!zver) + return false; + + /* Compare the major version number */ + if (atoi(version) <= atoi(zver)) + return true; + + return false; +} + +int tracecmd_zlib_init(void) +{ + if (zlib_handle) + return 0; + + zlib_handle = dlopen(__ZLIB_FILE, RTLD_NOW | RTLD_GLOBAL); + if (!zlib_handle) + return -1; + _lib_compress = dlsym(zlib_handle, ZLIB_FUNC_COMPRESS); + if (!_lib_compress) + goto error; + _libz_decompress = dlsym(zlib_handle, ZLIB_FUNC_DECOMOPRESS); + if (!_libz_decompress) + goto error; + _libz_compress_bound = dlsym(zlib_handle, ZLIB_FUNC_SIZE); + if (!_libz_compress_bound) + goto error; + _libz_version = dlsym(zlib_handle, ZLIB_FUNC_VERSION); + if (!_libz_version) + goto error; + + return tracecmd_compress_proto_register(__ZLIB_NAME, _libz_version(), __ZLIB_WEIGTH, + zlib_compress, zlib_decompress, + zlib_compress_bound, zlib_is_supported); + +error: + _lib_compress = NULL; + _libz_decompress = NULL; + _libz_version = NULL; + dlclose(zlib_handle); + zlib_handle = NULL; + return -1; +} + +void tracecmd_zlib_free(void) +{ + _lib_compress = NULL; + _libz_decompress = NULL; + _libz_version = NULL; + + if (zlib_handle) { + dlclose(zlib_handle); + zlib_handle = NULL; + } + +} diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c index 378a3e57..039f60c5 100644 --- a/lib/trace-cmd/trace-compress.c +++ b/lib/trace-cmd/trace-compress.c @@ -355,6 +355,10 @@ void tracecmd_compress_init(void) gettimeofday(&time, NULL); srand((time.tv_sec * 1000) + (time.tv_usec / 1000)); + +#ifdef HAVE_ZLIB + tracecmd_zlib_init(); +#endif } static struct compress_proto *compress_proto_select(void) @@ -531,6 +535,10 @@ void tracecmd_compress_free(void) struct compress_proto *proto = proto_list; struct compress_proto *del; +#ifdef HAVE_ZLIB + tracecmd_zlib_free(); +#endif + while (proto) { del = proto; proto = proto->next;
If libz is available, use that library to provide trace file compression support. The library is detected runtime. Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com> --- lib/trace-cmd/Makefile | 10 ++ lib/trace-cmd/include/trace-cmd-local.h | 5 + lib/trace-cmd/trace-compress-zlib.c | 172 ++++++++++++++++++++++++ lib/trace-cmd/trace-compress.c | 8 ++ 4 files changed, 195 insertions(+) create mode 100644 lib/trace-cmd/trace-compress-zlib.c