Re: [PATCH v4 01/10] cxl: add helper function to parse trace event to json object

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Tue, 2022-11-08 at 11:29 -0700, Dave Jiang wrote:
> Add the helper function that parses a trace event captured by
> libtraceevent in a tep handle. All the parsed fields are added to a json
> object. The json object is added to the provided list in the input parameter.
> 
> Tested-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c |  201 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/event_trace.h |   14 ++++
>  cxl/meson.build   |    2 +
>  meson.build       |    1 
>  4 files changed, 218 insertions(+)
>  create mode 100644 cxl/event_trace.c
>  create mode 100644 cxl/event_trace.h
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> new file mode 100644
> index 000000000000..1b1b037e48bf
> --- /dev/null
> +++ b/cxl/event_trace.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2022, Intel Corp. All rights reserved.
> +#include <stdio.h>
> +#include <json-c/json.h>
> +#include <util/json.h>
> +#include <util/util.h>
> +#include <util/parse-options.h>
> +#include <util/parse-configs.h>

The above two seem unnecessary.

> +#include <util/strbuf.h>
> +#include <util/sysfs.h>

Same with this.

> +#include <ccan/list/list.h>
> +#include <ndctl/ndctl.h>
> +#include <ndctl/libndctl.h>

Are the ndctl includes needed for anything?

> +#include <sys/epoll.h>
> +#include <sys/stat.h>

These two look unused too.

> +#include <libcxl.h>

This could be dropped too.

> +#include <uuid/uuid.h>
> +#include <traceevent/event-parse.h>
> +#include "json.h"
> +#include "event_trace.h"
> +
> +#define _GNU_SOURCE
> +#include <string.h>
> +
> +static struct json_object *num_to_json(void *num, int elem_size, unsigned long flags)
> +{
> +       bool sign = flags & TEP_FIELD_IS_SIGNED;
> +       int64_t val = 0;
> +
> +       /* special case 64 bit as the call depends on sign */
> +       if (elem_size == 8) {
> +               if (sign)
> +                       return json_object_new_int64(*(int64_t *)num);
> +               else
> +                       return json_object_new_uint64(*(uint64_t *)num);
> +       }
> +
> +       /* All others fit in a signed 64 bit */
> +       switch (elem_size) {
> +       case 1:
> +               if (sign)
> +                       val = *(int8_t *)num;
> +               else
> +                       val = *(uint8_t *)num;
> +               break;
> +       case 2:
> +               if (sign)
> +                       val = *(int16_t *)num;
> +               else
> +                       val = *(uint16_t *)num;
> +               break;
> +       case 4:
> +               if (sign)
> +                       val = *(int32_t *)num;
> +               else
> +                       val = *(uint32_t *)num;
> +               break;
> +       default:
> +               /*
> +                * Odd sizes are converted in the kernel to one of the above.
> +                * It is an error to see them here.
> +                */
> +               return NULL;
> +       }
> +
> +       return json_object_new_int64(val);
> +}
> +
> +static int cxl_event_to_json_callback(struct tep_event *event,
> +               struct tep_record *record, struct list_head *jlist_head)

Instead of the old double-tab style continuations, we added a .clang-
format to ndctl, which will format this slightly differently (matching
the continuation to the first char after the opening parens).

Also, is 'callback' in the name useful? I'd consider dropping it -
'cxl_event_to_json' conveys just as much information about what it
does.

> +{
> +       struct tep_format_field **fields;
> +       struct json_object *jevent, *jobj, *jarray;

Minor/optional nit, but these look nice and clean in reverse-christmas
tree used elsewhere :)

> +       struct jlist_node *jnode;
> +       int i, j, rc = 0;
> +
> +       jnode = malloc(sizeof(*jnode));
> +       if (!jnode)
> +               return -ENOMEM;
> +
> +       jevent = json_object_new_object();
> +       if (!jevent) {
> +               rc = -ENOMEM;
> +               goto err_jevent;

Also minor, but the label should probably be called err_free or
something since you goto it before jevent needs to be freed.

> +       }
> +       jnode->jobj = jevent;
> +
> +       fields = tep_event_fields(event);
> +       if (!fields) {
> +               rc = -ENOENT;
> +               goto err;
> +       }
> +
> +       jobj = json_object_new_string(event->system);
> +       if (!jobj) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +       json_object_object_add(jevent, "system", jobj);
> +
> +       jobj = json_object_new_string(event->name);
> +       if (!jobj) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +       json_object_object_add(jevent, "event", jobj);
> +
> +       jobj = json_object_new_uint64(record->ts);
> +       if (!jobj) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +       json_object_object_add(jevent, "timestamp", jobj);
> +
> +       for (i = 0; fields[i]; i++) {
> +               struct tep_format_field *f = fields[i];
> +               int len;
> +
> +               if (f->flags & TEP_FIELD_IS_STRING) {
> +                       char *str;
> +
> +                       str = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +                       if (!str)
> +                               continue;
> +
> +                       jobj = json_object_new_string(str);
> +                       if (!jobj) {
> +                               rc = -ENOMEM;
> +                               goto err;
> +                       }
> +
> +                       json_object_object_add(jevent, f->name, jobj);
> +               } else if (f->flags & TEP_FIELD_IS_ARRAY) {
> +                       unsigned char *data;
> +                       int chunks;
> +
> +                       data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +                       if (!data)
> +                               continue;
> +
> +                       jarray = json_object_new_array();
> +                       if (!jarray) {
> +                               rc = -ENOMEM;
> +                               goto err;
> +                       }
> +
> +                       chunks = f->size / f->elementsize;
> +                       for (j = 0; j < chunks; j++) {
> +                               jobj = num_to_json(data, f->elementsize, f->flags);
> +                               if (!jobj) {
> +                                       json_object_put(jarray);
> +                                       return -ENOMEM;
> +                               }
> +                               json_object_array_add(jarray, jobj);
> +                               data += f->elementsize;
> +                       }
> +
> +                       json_object_object_add(jevent, f->name, jarray);
> +               } else { /* single number */
> +                       unsigned char *data;
> +                       char *tmp;
> +
> +                       data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +                       if (!data)
> +                               continue;
> +
> +                       /* check to see if we have a UUID */
> +                       tmp = strcasestr(f->type, "uuid_t");
> +                       if (tmp) {
> +                               char uuid[SYSFS_ATTR_SIZE];

Does this need to be SYSFS_ATTR_SIZE? Looks like historically we've
just created such UUID buffers to be buf[40]. I think the sysfs size is
4096.

> +
> +                               uuid_unparse(data, uuid);
> +                               jobj = json_object_new_string(uuid);
> +                               if (!jobj) {
> +                                       rc = -ENOMEM;
> +                                       goto err;
> +                               }
> +
> +                               json_object_object_add(jevent, f->name, jobj);
> +                               continue;
> +                       }
> +
> +                       jobj = num_to_json(data, f->elementsize, f->flags);
> +                       if (!jobj) {
> +                               rc = -ENOMEM;
> +                               goto err;
> +                       }
> +
> +                       json_object_object_add(jevent, f->name, jobj);
> +               }
> +       }
> +
> +       list_add_tail(jlist_head, &jnode->list);
> +       return 0;
> +
> +err:
> +       json_object_put(jevent);
> +err_jevent:
> +       free(jnode);
> +       return rc;
> +}
> diff --git a/cxl/event_trace.h b/cxl/event_trace.h
> new file mode 100644
> index 000000000000..00975a0b5680
> --- /dev/null
> +++ b/cxl/event_trace.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2022 Intel Corporation. All rights reserved. */
> +#ifndef __CXL_EVENT_TRACE_H__
> +#define __CXL_EVENT_TRACE_H__
> +
> +#include <json-c/json.h>
> +#include <ccan/list/list.h>
> +
> +struct jlist_node {
> +       struct json_object *jobj;
> +       struct list_node list;
> +};
> +
> +#endif
> diff --git a/cxl/meson.build b/cxl/meson.build
> index f2474aaa6e2e..8c7733431613 100644
> --- a/cxl/meson.build
> +++ b/cxl/meson.build
> @@ -7,6 +7,7 @@ cxl_src = [
>    'memdev.c',
>    'json.c',
>    'filter.c',
> +  'event_trace.c',
>  ]
>  
>  cxl_tool = executable('cxl',
> @@ -19,6 +20,7 @@ cxl_tool = executable('cxl',
>      kmod,
>      json,
>      versiondep,
> +    traceevent,
>    ],
>    install : true,
>    install_dir : rootbindir,
> diff --git a/meson.build b/meson.build
> index 20a646d135c7..f611e0bdd7f3 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -142,6 +142,7 @@ kmod = dependency('libkmod')
>  libudev = dependency('libudev')
>  uuid = dependency('uuid')
>  json = dependency('json-c')
> +traceevent = dependency('libtraceevent')
>  if get_option('docs').enabled()
>    if get_option('asciidoctor').enabled()
>      asciidoc = find_program('asciidoctor', required : true)
> 
>