On Wed, Aug 14, 2024 at 23:40:26 +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 <[email protected]>
> ---
> tools/nss/libvirt_nss_macs.c | 263 +++++++++--------------------------
> 1 file changed, 68 insertions(+), 195 deletions(-)
>
> diff --git a/tools/nss/libvirt_nss_macs.c b/tools/nss/libvirt_nss_macs.c
> index f45d149793..dbc0790945 100644
> --- a/tools/nss/libvirt_nss_macs.c
> +++ b/tools/nss/libvirt_nss_macs.c
> @@ -25,179 +25,78 @@
> #include <stdlib.h>
> #include <fcntl.h>
>
> -#include <yajl/yajl_gen.h>
> -#include <yajl/yajl_parse.h>
> +#include <json.h>
Same issue with linking as outlined in previous patch.
>
> #include "libvirt_nss_macs.h"
> #include "libvirt_nss.h"
>
> -enum {
> - FIND_MACS_STATE_START,
> - FIND_MACS_STATE_LIST,
> - FIND_MACS_STATE_ENTRY,
> - FIND_MACS_STATE_ENTRY_MACS,
> -};
> -
> -typedef struct {
> - const char *name;
> - char ***macs;
> - size_t *nmacs;
> - int state;
> -
> - char *key;
> - struct {
> - char *name;
> - char **macs;
> - size_t nmacs;
> - } entry;
> -} findMACsParser;
> -
>
> static int
> -findMACsParserString(void *ctx,
> - const unsigned char *stringVal,
> - size_t stringLen)
As before please document this.
> +findMACsFromJSON(json_object *jobj,
> + const char *name,
> + char ***macs,
> + size_t *nmacs)
> {
> - findMACsParser *parser = ctx;
> -
> - 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 (parser->state == FIND_MACS_STATE_ENTRY) {
> - if (strcmp(parser->key, "domain"))
> - return 1;
> -
> - free(parser->entry.name);
> - if (!(parser->entry.name = strndup((char *)stringVal, stringLen)))
> - return 0;
> - } else if (parser->state == FIND_MACS_STATE_ENTRY_MACS) {
> - char **macs;
> - if (strcmp(parser->key, "macs"))
> - return 1;
> + size_t i, j;
One per line.
> + int len;
>
> - if (!(macs = realloc(parser->entry.macs,
> - sizeof(char *) * (parser->entry.nmacs + 1))))
> - return 0;
> -
> - parser->entry.macs = macs;
> - if (!(macs[parser->entry.nmacs++] = strndup((char *)stringVal,
> stringLen)))
> - return 0;
> - } 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;
> -}
> -
> -
> -static int
> -findMACsParserMapKey(void *ctx,
> - const unsigned char *stringVal,
> - size_t stringLen)
> -{
> - findMACsParser *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
> -findMACsParserStartMap(void *ctx)
> -{
> - findMACsParser *parser = ctx;
>
> - DEBUG("Parse start map state=%d", parser->state);
> + len = json_object_array_length(jobj);
> + DEBUG("Found an array of length: %zu", len);
> + for (i = 0; i < len; i++) {
> + json_object *entry = NULL;
> + json_object *domain = NULL;
> + const char *domainName;
> + char **tmpMacs = NULL;
> + size_t newmacs = 0;
> + json_object *macsArray = NULL;
>
> - if (parser->state != FIND_MACS_STATE_LIST)
> - return 0;
> + entry = json_object_array_get_idx(jobj, i);
> + if (!entry)
> + return -1;
>
> - free(parser->key);
> - parser->key = NULL;
> - parser->state = FIND_MACS_STATE_ENTRY;
> + DEBUG("Processing item %zu", i);
>
> - return 1;
> -}
> + domain = json_object_object_get(entry, "domain");
> + if (!domain)
> + return -1;
>
> + domainName = json_object_get_string(domain);
> + if (!domainName)
> + return -1;
No error reporting in any of these? If no shouldn't this continue
processing the loop? (also below)
>
> -static int
> -findMACsParserEndMap(void *ctx)
> -{
> - findMACsParser *parser = ctx;
> - size_t i;
> + DEBUG("Processing domain %s", domainName);
>
> - DEBUG("Parse end map state=%d", parser->state);
> + if (strcasecmp(domainName, name))
> + continue;
>
> - if (parser->entry.name == NULL)
> - return 0;
> + macsArray = json_object_object_get(entry, "macs");
> + if (!macsArray)
> + return -1;
>
> - if (parser->state != FIND_MACS_STATE_ENTRY)
> - return 0;
> + newmacs = json_object_array_length(macsArray);
> + DEBUG("Found %zu MAC addresses", newmacs);
>
> - if (!strcasecmp(parser->entry.name, parser->name)) {
> - char **macs = realloc(*parser->macs,
> - sizeof(char *) * ((*parser->nmacs) +
> parser->entry.nmacs));
> - if (!macs)
> + tmpMacs = realloc(*macs, sizeof(char *) * (*nmacs + newmacs + 1));
> + if (!tmpMacs)
> return 0;
So allocation failure is success?
>
> - *parser->macs = macs;
> - for (i = 0; i < parser->entry.nmacs; i++)
> - (*parser->macs)[(*parser->nmacs)++] = parser->entry.macs[i];
> - } else {
> - for (i = 0; i < parser->entry.nmacs; i++)
> - free(parser->entry.macs[i]);
> - }
> - free(parser->entry.macs);
> - parser->entry.macs = NULL;
> - parser->entry.nmacs = 0;
> -
> - parser->state = FIND_MACS_STATE_LIST;
> -
> - return 1;
> -}
> -
> -
> -static int
> -findMACsParserStartArray(void *ctx)
> -{
> - findMACsParser *parser = ctx;
> -
> - DEBUG("Parse start array state=%d", parser->state);
> -
> - if (parser->state == FIND_MACS_STATE_START)
> - parser->state = FIND_MACS_STATE_LIST;
> - else if (parser->state == FIND_MACS_STATE_ENTRY)
> - parser->state = FIND_MACS_STATE_ENTRY_MACS;
> - else
> - return 0;
> + *macs = tmpMacs;
>
> - return 1;
> -}
> + for (j = 0; j < newmacs; j++) {
> + json_object *macobj = NULL;
>
> + DEBUG("j: %zu", j);
This doesn't sound very useful.
>
> -static int
> -findMACsParserEndArray(void *ctx)
> -{
> - findMACsParser *parser = ctx;
> -
> - DEBUG("Parse end array state=%d", parser->state);
> -
> - if (parser->state == FIND_MACS_STATE_LIST)
> - parser->state = FIND_MACS_STATE_START;
> - else if (parser->state == FIND_MACS_STATE_ENTRY_MACS)
> - parser->state = FIND_MACS_STATE_ENTRY;
> - else
> - return 0;
> -
> - return 1;
> + macobj = json_object_array_get_idx(macsArray, j);
> + (*macs)[(*nmacs)++] = strdup(json_object_get_string(macobj));
No return value check from strdup (since you are checking realloc).
> + }
> + }
> + return 0;
> }
>
>
> @@ -209,66 +108,47 @@ findMACs(const char *file,
> {
> int fd = -1;
> int ret = -1;
> - const yajl_callbacks parserCallbacks = {
> - NULL, /* null */
> - NULL, /* bool */
> - NULL, /* integer */
> - NULL, /* double */
> - NULL, /* number */
> - findMACsParserString,
> - findMACsParserStartMap,
> - findMACsParserMapKey,
> - findMACsParserEndMap,
> - findMACsParserStartArray,
> - findMACsParserEndArray,
> - };
> - findMACsParser parserState = {
> - .name = name,
> - .macs = macs,
> - .nmacs = nmacs,
> - };
> - yajl_handle parser = NULL;
> char line[1024];
> - size_t i;
> + json_object *jobj = NULL;
> + json_tokener *tok = NULL;
> + enum json_tokener_error jerr = json_tokener_success;
> + int jsonflags = JSON_TOKENER_STRICT | JSON_TOKENER_VALIDATE_UTF8;
> + ssize_t nreadTotal = 0;
> int rv;
> + size_t i;
>
> if ((fd = open(file, O_RDONLY)) < 0) {
> ERROR("Cannot open %s", 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) {
> + 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;
> +
> + jobj = json_tokener_parse_ex(tok, line, rv);
> + jerr = json_tokener_get_error(tok);
> }
>
> - if (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;
> }
>
> - ret = 0;
> + ret = findMACsFromJSON(jobj, name, macs, nmacs);
Same issues with the parser as in patch before.
