On Wed, Sep 27, 2023 at 06:52:34PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 27, 2023 at 04:41:18PM +0200, Phil Sutter wrote:
> > On Wed, Sep 27, 2023 at 03:09:53PM +0200, Phil Sutter wrote:
> > > On Wed, Sep 27, 2023 at 01:19:31PM +0200, Pablo Neira Ayuso wrote:
> > > > Hi Phil,
> > > >
> > > > On Wed, Sep 27, 2023 at 01:10:09PM +0200, Phil Sutter wrote:
> [...]
> > > > > I actually considered forking the project. Or we just ship a copy of the
> > > > > lib with nftables sources?
> > > >
> > > > I would try to get back to them to refresh and retry.
> > >
> > > Oh well. I'll try an approach which eliminates the pointer if not
> > > enabled. The terse feedback and pessimistic replies right from the start
> > > convinced me though they just don't want it.
> >
> > OK, so I had a close look at the code and played a bit with pahole. My
> > approach to avoiding the extra pointer is to add another set of types
> > which json_t embed. So taking json_array_t as an example:
> >
> > | typedef struct {
> > | json_t json;
> > | size_t size;
> > | size_t entries;
> > | json_t **table;
> > | } json_array_t;
> >
> > I could introduce json_location_array_t:
> >
> > | typedef struct {
> > | json_array_t array;
> > | json_location_t *location;
> > | } json_location_array_t;
> >
> > The above structs are opaque to users, they only know about json_t.
>
> OK, so this new object type is hiding behind the json_t opaque type.
>
> > So I introduced a getter for the location data:
> >
> > | int json_get_location(json_t *json, int *line, int *column,
> > | int *position, int *length);
> >
> > In there, I have to map from json_t to the type in question. The problem
> > is to know whether I have a json_location_array_t or just a
> > json_array_t. The json_t may have been allocated by the input parser
> > with either JSON_STORE_LOCATION set or not or by json_array().
> >
> > In order to make the decision, I need at least a bit in well-known
> > memory. Pahole tells there's a 4byte hole in json_t, but it may be
> > gone in 32bit builds (and enlarging json_t is a no-go, they consider
> > it ABI). The json_*_t structures don't show any holes, and extending
> > them means adding a mandatory word due to buffering, so I may just
> > as well store the location pointer itself in them.
> >
> > The only feasible alternative is to store location data separate from
> > the objects themselves, ideally in a hash table. This reduces the
> > overhead if not used by a failing hash table lookup in json_delete().
>
> If I understood correctly, then this means you are ditching the
> json_location_array_t approach that you are detailing above.
>
> The hashtable approach might be sensible to follow, and such approach
> does not require any update to libjansson?
>
It does! We can't access the parser state from outside and during the
parsing of input data. The whole thing has to reside within libjansson.
Here's my reimplementation:
https://github.com/akheron/jansson/pull/662
Any review and/or supportive comment highly appreciated. (:
Cheers, Phil
