Re: [libvirt PATCHv2 10/15] nss: convert findLeases to use json-c

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Friday in 2024, Peter Krempa wrote:
> On Thu, Sep 05, 2024 at 15:49:37 +0200, Ján Tomko wrote:
> > While the parsing is still done by 1K buffers, the results
> > are no longer filtered during the parsing, but the whole JSON
> > has to live in memory at once, which was also the case before
> > the NSS plugin dropped its dependency on libvirt_util.
> >
> > Also, the new parser might be more forgiving of missing elements.
> >
> > Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> > ---
> >  tools/nss/libvirt_nss_leases.c | 370 +++++++++++----------------------
> >  1 file changed, 119 insertions(+), 251 deletions(-)

> > +/**
> > + * findLeaseInJSON
> > + *
> > + * @jobj: the json object containing the leases
> > + * @name: the requested hostname (optional if a MAC address is present)
> > + * @macs: the array of MAC addresses we're matching (optional if we have a hostname)
> > + * @nmacs: the size of the MAC array
> > + * @af: the requested address family
> > + * @now: current time (to eliminate expired leases)
> > + * @addrs: the returned matching addresses
> > + * @naddrs: size of the returned array
> > + * @found: whether a match was found
> > + *
> > + * Returns 0 even if nothing was found
> > + *        -1 on error
> > + */
> >  static int
> > +findLeaseInJSON(json_object *jobj,
> > +                const char *name,
> > +                char **macs,
> > +                size_t nmacs,
> > +                int af,
> > +                time_t now,
> > +                leaseAddress **addrs,
> > +                size_t *naddrs,
> > +                bool *found)
> >  {
> >      size_t i;
> > +    int len;
> >
> > +    if (!json_object_is_type(jobj, json_type_array)) {
> > +        ERROR("parsed JSON does not contain the leases array");
> > +        return -1;
> >      }
> >
> > +    len = json_object_array_length(jobj);
> > +    for (i = 0; i < len; i++) {
> > +        json_object *lease = NULL;
> > +        json_object *expiry = NULL;
> > +        json_object *ipobj = NULL;
> > +        unsigned long long expiryTime;
> > +        const char *ipaddr;
> > +
> > +        lease = json_object_array_get_idx(jobj, i);
> > +
> > +        if (macs) {
> > +            const char *macAddr;
> > +            bool match = false;
> > +            json_object *val;
> > +            size_t j;
> > +
> > +            val = json_object_object_get(lease, "mac-address");
> > +            if (!val)
> > +                continue;
> > +
> > +            macAddr = json_object_get_string(val);
> > +            if (!macAddr)
> > +                continue;
> > +
> > +            for (j = 0; j < nmacs; j++) {
> > +                if (!strcmp(macs[j], macAddr)) {
>
> strcmp(..) != 0

I assume this was just a style comment, not about the logic, i.e.
strcmp(..) == 0

> > +                    match = true;
> > +                    break;
> > +                }
> > +            }
> > +            if (!match)
> > @@ -389,51 +271,37 @@ findLeases(const char *file,
> >          goto cleanup;
> >      }
> >
> > +    tok = json_tokener_new();
> > +    json_tokener_set_flags(tok, jsonflags);
> >
> > +    do {
> > +        rv = read(fd, line, sizeof(line) - 1);
>
> -1 should not be needed here
>
> >          if (rv < 0)
> >              goto cleanup;
> >          if (rv == 0)
> >              break;
>
> So if you get an incomplete JSON document of exactly sizeof(len) bytes,
> this will go through the first loop adn thend break out ...
>
> >          nreadTotal += rv;
> >
> > +        jobj = json_tokener_parse_ex(tok, line, rv);
> > +        jerr = json_tokener_get_error(tok);
> > +    } while (jerr == json_tokener_continue);

I've added:

    if (jerr == json_tokener_continue) {
        ERROR("Cannot parse %s: incomplete json found", file);
        goto cleanup;
    }

Jano

> > +    if (nreadTotal > 0 && jerr != json_tokener_success) {
>
> ... this condition will match as we read something and 'jerr' is still
>
> 'json_tokener_continue'
>
> > +        ERROR("Cannot parse %s: %s", file, json_tokener_error_desc(jerr));
>
> So this error will read:
>
>  Cannot parse /path/to/file: continue

Attachment: signature.asc
Description: PGP signature