On Tue, 22 Feb 2022 05:42:56 +0200
Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
> > >
> > > 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.
The way this is done in the kernel interface (aka. system calls) is to
pass a structure along with the size. That is, the API will know which
version of the API is being called by the size of the structure.
Anything new will have to be appended to the structure, which will
increase it. If the library receives a structure of a smaller size, but
one that was once supported, it could use that information to just do
the old method. In other words, only use the fields of a structure that
fit into the size given.
>
> >
> > 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 ;)
Oh, we are always touching errno ;-)
>
> 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.
I'm hoping to get everything merged this week, but probably will not
release trace-cmd until next week. I'm still sorting out the libraries.
But I'm hoping that they are almost done.
-- Steve
|