On Wed, Aug 14, 2024 at 23:40:25 +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 | 339 ++++++++++-----------------------
> 1 file changed, 96 insertions(+), 243 deletions(-)
>
> diff --git a/tools/nss/libvirt_nss_leases.c b/tools/nss/libvirt_nss_leases.c
> index 770dae8625..86bf184ea5 100644
> --- a/tools/nss/libvirt_nss_leases.c
> +++ b/tools/nss/libvirt_nss_leases.c
> @@ -26,39 +26,11 @@
> #include <stdbool.h>
> #include <fcntl.h>
>
> -#include <yajl/yajl_gen.h>
> -#include <yajl/yajl_parse.h>
> +#include <json.h>
So at this point meson will try to build this based on whether yajl is
present but this fully switches to json-c. I think you'll need to merge
in both the link statements and also the meson changes to ensure this is
compiled correctly.
Obviously that's in the case when you don't want to have two
side-by-side impls as we have in util/virjson.c at this point.
>
> #include "libvirt_nss_leases.h"
> #include "libvirt_nss.h"
>
> -enum {
> - FIND_LEASES_STATE_START,
> - FIND_LEASES_STATE_LIST,
> - FIND_LEASES_STATE_ENTRY,
> -};
> -
> -
> -typedef struct {
> - const char *name;
> - char **macs;
> - size_t nmacs;
> - int state;
> - unsigned long long now;
> - int af;
> - bool *found;
> - leaseAddress **addrs;
> - size_t *naddrs;
> -
> - char *key;
> - struct {
> - unsigned long long expiry;
> - char *ipaddr;
> - char *macaddr;
> - char *hostname;
> - } entry;
> -} findLeasesParser;
> -
>
> static int
> appendAddr(const char *name __attribute__((unused)),
> @@ -157,189 +129,102 @@ appendAddr(const char *name __attribute__((unused)),
>
>
> static int
> -findLeasesParserInteger(void *ctx,
> - long long val)
Please document this function/arguments so that it's easier to
understand for the next person.
Yes I do realize that the current version is absolutely undocumented.
> +findLeaseInJSON(json_object *jobj,
> + const char *name,
> + char **macs,
> + size_t nmacs,
> + int af,
> + time_t now,
> + leaseAddress **addrs,
> + size_t *naddrs,
> + bool *found)
Uhh, this diff will be very hard to review, based on how useless git
decided to make this diff..
I've resorted to looking just at the new code after this patch mostly
and will try to find the matching code to comment.
> {
> - findLeasesParser *parser = ctx;
> + size_t i, j;
One per line. Also 'j' is used only in one of the innter blocks, so
declare it there?
> + int len;
>
> - DEBUG("Parse int state=%d '%lld' (map key '%s')",
> - parser->state, val, NULLSTR(parser->key));
> - if (!parser->key)
> - return 0;
> -
> - if (parser->state == FIND_LEASES_STATE_ENTRY) {
> - if (strcmp(parser->key, "expiry-time"))
> - return 0;
> -
> - parser->entry.expiry = val;
> - } else {
> - return 0;
> + if (!json_object_is_type(jobj, json_type_array)) {
> + ERROR("parsed JSON does not contain the leases array");
> + return -1;
> }
> - 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;
>
> -static int
> -findLeasesParserString(void *ctx,
> - const unsigned char *stringVal,
> - size_t stringLen)
> -{
> - findLeasesParser *parser = ctx;
> + lease = json_object_array_get_idx(jobj, i);
>
> - DEBUG("Parse string state=%d '%.*s' (map key '%s')",
> - parser->state, (int)stringLen, (const char *)stringVal,
> - NULLSTR(parser->key));
> - if (!parser->key)
> - return 0;
> + if (macs) {
> + const char *macAddr;
> + bool match = false;
> + json_object *val;
>
> - if (parser->state == FIND_LEASES_STATE_ENTRY) {
> - if (!strcmp(parser->key, "ip-address")) {
> - if (!(parser->entry.ipaddr = strndup((char *)stringVal, stringLen)))
> - return 0;
> - } else if (!strcmp(parser->key, "mac-address")) {
> - if (!(parser->entry.macaddr = strndup((char *)stringVal, stringLen)))
> - return 0;
> - } else if (!strcmp(parser->key, "hostname")) {
> - if (!(parser->entry.hostname = strndup((char *)stringVal, stringLen)))
> - return 0;
> + 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)) {
> + match = true;
> + break;
> + }
> + }
> + if (!match)
> + continue;
> } else {
> - return 1;
> - }
> - } else {
> - return 0;
> - }
> - return 1;
> -}
> -
> -
> -static int
> -findLeasesParserMapKey(void *ctx,
> - const unsigned char *stringVal,
> - size_t stringLen)
> -{
> - findLeasesParser *parser = ctx;
> -
> - DEBUG("Parse map key state=%d '%.*s'",
> - parser->state, (int)stringLen, (const char *)stringVal);
> -
> - free(parser->key);
> - if (!(parser->key = strndup((char *)stringVal, stringLen)))
> - return 0;
> -
> - return 1;
> -}
> -
> -
> -static int
> -findLeasesParserStartMap(void *ctx)
> -{
> - findLeasesParser *parser = ctx;
> -
> - DEBUG("Parse start map state=%d", parser->state);
> -
> - if (parser->state != FIND_LEASES_STATE_LIST)
> - return 0;
> -
> - free(parser->key);
> - parser->key = NULL;
> - parser->state = FIND_LEASES_STATE_ENTRY;
> -
> - return 1;
> -}
> -
> -
> -static int
> -findLeasesParserEndMap(void *ctx)
> -{
> - findLeasesParser *parser = ctx;
> - size_t i;
> - bool found = false;
> -
> - DEBUG("Parse end map state=%d", parser->state);
> + const char *leaseName;
> + json_object *val;
>
> - if (parser->entry.macaddr == NULL)
> - return 0;
> + val = json_object_object_get(lease, "hostname");
> + if (!val)
> + continue;
>
> - if (parser->state != FIND_LEASES_STATE_ENTRY)
> - return 0;
> + leaseName = json_object_get_string(val);
> + if (!leaseName)
> + continue;
>
> - if (parser->nmacs) {
> - DEBUG("Check %zu macs", parser->nmacs);
> - for (i = 0; i < parser->nmacs && !found; i++) {
> - DEBUG("Check mac '%s' vs '%s'", parser->macs[i], NULLSTR(parser->entry.macaddr));
> - if (parser->entry.macaddr && !strcmp(parser->macs[i], parser->entry.macaddr))
> - found = true;
> + if (strcasecmp(leaseName, name))
> + continue;
So this looks broken. You skip the processing if the requested name
*doesn't equal* to the lease name.
I very strongly suggest adding explicit comparison with 0 for str*cmp.
> }
> - } else {
> - DEBUG("Check name '%s' vs '%s'", parser->name, NULLSTR(parser->entry.hostname));
> - if (parser->entry.hostname && !strcasecmp(parser->name, parser->entry.hostname))
> - found = true;
> - }
> - DEBUG("Found %d", found);
> - if (parser->entry.expiry != 0 &&
> - parser->entry.expiry < parser->now) {
> - DEBUG("Entry expired at %llu vs now %llu",
> - parser->entry.expiry, parser->now);
> - found = false;
> - }
> - if (!parser->entry.ipaddr)
> - found = false;
> -
> - if (found) {
> - *parser->found = true;
> -
> - if (appendAddr(parser->name,
> - parser->addrs, parser->naddrs,
> - parser->entry.ipaddr,
> - parser->entry.expiry,
> - parser->af) < 0)
> - return 0;
> - }
> -
> - free(parser->entry.macaddr);
> - free(parser->entry.ipaddr);
> - free(parser->entry.hostname);
> - parser->entry.expiry = 0;
> - parser->entry.macaddr = NULL;
> - parser->entry.ipaddr = NULL;
> - parser->entry.hostname = NULL;
>
> - parser->state = FIND_LEASES_STATE_LIST;
> -
> - return 1;
> -}
> + expiry = json_object_object_get(lease, "expiry-time");
> + if (!expiry) {
> + ERROR("Missing expiry time for %s", name);
> + return -1;
> + }
>
> + expiryTime = json_object_get_uint64(expiry);
> + if (expiryTime && expiryTime < now) {
since expiryTime is a number and you are checking that it's non-zero,
please use explicit check so that it's obvious.
> + DEBUG("Skipping expired lease for %s", name);
> + continue;
> + }
>
> -static int
> -findLeasesParserStartArray(void *ctx)
> -{
> - findLeasesParser *parser = ctx;
> + ipobj = json_object_object_get(lease, "ip-address");
> + if (!ipobj) {
> + DEBUG("Missing IP address for %s", name);
> + continue;
> + }
> + ipaddr = json_object_get_string(ipobj);
>
> - DEBUG("Parse start array state=%d", parser->state);
> + DEBUG("Found record for %s", name);
> + *found = true;
>
> - if (parser->state == FIND_LEASES_STATE_START) {
> - parser->state = FIND_LEASES_STATE_LIST;
> - } else {
> - return 0;
> + if (appendAddr(name,
> + addrs, naddrs,
> + ipaddr,
> + expiryTime,
> + af) < 0)
> + return -1;
> }
>
> - return 1;
> -}
> -
> -
> -static int
> -findLeasesParserEndArray(void *ctx)
> -{
> - findLeasesParser *parser = ctx;
> -
> - DEBUG("Parse end array state=%d", parser->state);
> -
> - if (parser->state == FIND_LEASES_STATE_LIST)
> - parser->state = FIND_LEASES_STATE_START;
> - else
> - return 0;
> -
> - return 1;
> + return 0;
> }
>
>
> @@ -356,30 +241,10 @@ findLeases(const char *file,
[...]
> + json_object *jobj = NULL;
> + json_tokener *tok = NULL;
> + enum json_tokener_error jerr = json_tokener_success;
You set this here to 'json_tokener_success'
> + int jsonflags = JSON_TOKENER_STRICT | JSON_TOKENER_VALIDATE_UTF8;
> char line[1024];
> ssize_t nreadTotal = 0;
> int rv;
> @@ -389,51 +254,39 @@ findLeases(const char *file,
> goto cleanup;
> }
>
> - parser = yajl_alloc(&parserCallbacks, NULL, &parserState);
> - if (!parser) {
> - ERROR("Unable to create JSON parser");
> - goto cleanup;
> - }
> + tok = json_tokener_new();
> + json_tokener_set_flags(tok, jsonflags);
>
> - while (1) {
> - rv = read(fd, line, sizeof(line));
> + while (jerr != json_tokener_continue) {
So this passes on first iteration, but ...
> + rv = read(fd, line, sizeof(line) - 1);
> if (rv < 0)
> goto cleanup;
> if (rv == 0)
> break;
> nreadTotal += rv;
>
> - if (yajl_parse(parser, (const unsigned char *)line, rv) !=
> - yajl_status_ok) {
> - unsigned char *err = yajl_get_error(parser, 1,
> - (const unsigned char*)line, rv);
> - ERROR("Parse failed %s", (const char *) err);
> - yajl_free_error(parser, err);
> - goto cleanup;
> - }
> + line[rv] = 0;
Why is this needed ...
> +
> + jobj = json_tokener_parse_ex(tok, line, rv);
... this seems to accept a length argument.
> + jerr = json_tokener_get_error(tok);
... the docs state:
A partial JSON string can be parsed. If the parsing is incomplete,
NULL will be returned and json_tokener_get_error() will be return
json_tokener_continue. json_tokener_parse_ex() can then be called with
additional bytes in str to continue the parsing.
So unless I'm missing something very important this will not be able to
parse a file larger than 1k.
> }
>
> - if (nreadTotal > 0 &&
> - yajl_complete_parse(parser) != yajl_status_ok) {
> - ERROR("Parse failed %s",
> - yajl_get_error(parser, 1, NULL, 0));
> + if (nreadTotal > 0 && jerr != json_tokener_success) {
> + ERROR("Cannot parse %s: %s", file, json_tokener_error_desc(jerr));
> goto cleanup;
> }
