On Mon, Feb 21, 2022 at 3:55 PM Sebastian Andrzej Siewior
<sebastian@xxxxxxxxxxxxx> wrote:
>
[ ... ]
> > > +int tracecmd_zstd_init(void)
> > > +{
> > > + int ret = 0;
> > > + size_t r;
> > > +
> > > + 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(__ZSTD_NAME,
> > > + ZSTD_versionString(),
> > > + __ZSTD_WEIGTH,
> > > + zstd_compress,
> > > + zstd_decompress,
> > > + zstd_compress_bound,
> > > + zstd_is_supported);
> >
> > I think tracecmd_compress_proto_register() should be extend with two new hooks:
> > void *(*new_context)(void),
> > void (*free_context)(void *),
> > and ctx_c, ctx_d should be allocated and freed there. Currently, that
> > logic in trace-cmd is in a single thread. That may change in the
> > future, and then the current design will be a limitation.
>
> I would suggest to use
>
> struct tracecmd_compress_algo {
> const char *name,
> const char *version,
> int weight,
> func_t *compress;
> func_t *decompress;
> func_t *bound;
> func_t *supported;
> };
>
> and then
> ret = tracecmd_compress_proto_register(&comp_algo);
>
> so that the function has less args and can be extended without touching
> every algo.
We prefer to use explicit API arguments, to ensure that on API's
change the old callers of the API will be updated. Using such
structure is more flexible, but even though in most cases the change
will be backward compatible - it could be hard to debug the legacy
code in case the API's behaviour changes because of adding some new
parameter in the structure.
>
> As for func_t / calling convetion I would prefer something that is not
> that close to zlib. Like
>
> int comp(void *ctx, const void *src, signed int in_size,
> void *dst, signed int out_size);
>
> This clearly limits the sizes to 31 bit (with the 40kiB probably okay)
> and the return value can be either >= 0 returning the number of bytes
> produced or negative for an error. And please no errno touching ;)
>
I like the idea to make these hooks more generic, not so close to a
specific library. But there should be some error reporting mechanism,
why not to use errno ? In the best case, the library itself should set
the errno. But zlib has its own error codes, and according to its
documentation not all of the APIs set errno in case of an error.
That's why I added this explicit setting of errno in case of some
Z_ERROR.
Are you interested to submit a patch with these suggestions ? I also
can do it, when Steven merges zstd support and before the next
trace-cmd release.
> Sebastian
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|