Re: [ndctl PATCH 2/2] cxl: Add list image, image-from-file to CXL command

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

On Fri, 2022-08-19 at 01:57 -0700, sunfishho12@gmail.com wrote:
> From: Matthew Ho <sunfishho12@gmail.com>
> 
> This adds the option of creating a diagram of a CXL topology.
> Use cxl list -vvv --image --output-file=topology.png to 
> generate a diagram in topology.png.
> 
> Likewise, use cxl list --image-from-file=topology-txt
> to generate a diagram using topology.txt, where 
> topology.txt was generated by cxl list -vvv beforehand.
> 
> This addition makes it easier to visualize the CXL topology.
> Also, being able to choose to use an input text file 
> allows one to visualize the CXL topology while not having 
> access to CXL hardware, which may be convenient.
> 
> Signed-off-by: Matthew Ho <sunfishho12@gmail.com>
> ---
>  Documentation/cxl/cxl-list.txt |  16 ++
>  cxl/filter.c                   | 279 ++++++++++++++++++++++++++++++++-
>  cxl/filter.h                   |   7 +
>  cxl/list.c                     |  24 +++
>  cxl/meson.build                |   1 +
>  meson.build                    |   1 +
>  6 files changed, 327 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/cxl/cxl-list.txt b/Documentation/cxl/cxl-list.txt
> index 698f53b7a9e9..bd39889bbe3a 100644
> --- a/Documentation/cxl/cxl-list.txt
> +++ b/Documentation/cxl/cxl-list.txt
> @@ -354,6 +354,22 @@ OPTIONS
>           Everything *-vv* provides, plus enable
>           --health and --partition.
>  
> +--image::
> +-o::
> +--output-file::

So listing the three this way makes it seem they're the same option
when they are not. I suggest doing the options this way instead:

-o::
--output=::
      Output to a file. Format is decided based on file extension.
      .jpg / .png / .svg: output an image in the specified format with
      the CXL topology.
      .txt / .log / no extension: output a text file containing the
      JSON output
      Any other extension - reserved / unsupported.
      
And with this, drop the --image option entirely, as it can be deduced from
the output format.

> +       Create an image displaying the topology. One can specify an
> +       output with -o or --output-file, which supports outputting
> +       with file types .png, .jpg, or .jpeg. If -o is not specified,
> +       cxl list --image will by default create a new file topology.jpg
> +       to output to.
> +
> +--image-from-file::

Similarly this can just be --input=<file>
You can clarify in the description that the input file needs to be a
plaintext file only, i.e. image-to-text won't be supported.

> +       This subcommand has the same function as --image, except it
> +       requires an input text file to be specified that contains a
> +       json array outputted by cxl list. As with --image, one can
> +       use -o or --output-file to specify an output file, with the
> +       default behavior being to create and output to topology.jpg.
> +
>  --debug::
>         If the cxl tool was built with debug enabled, turn on debug
>         messages.
> diff --git a/cxl/filter.c b/cxl/filter.c
> index 38ece5528794..0ebf62960336 100644
> --- a/cxl/filter.c
> +++ b/cxl/filter.c
> @@ -8,6 +8,7 @@
>  #include <util/json.h>
>  #include <cxl/libcxl.h>
>  #include <json-c/json.h>
> +#include <graphviz/gvc.h>
>  
>  #include "filter.h"
>  #include "json.h"
> @@ -1004,6 +1005,279 @@ walk_children:
>         }
>  }
>  
> +static char *find_device_type(struct json_object *device)
> +{
> +       char *value;
> +
> +       json_object_object_foreach(device, property, value_json) {
> +               value = (char *)json_object_get_string(value_json);
> +               if (!strcmp(property, "bus") &&
> +                   !strcmp(value, "root0"))
> +                       return "ACPI0017 Device";
> +               if (!strcmp(property, "host") &&
> +                   !strncmp(value, "ACPI0016", strlen("ACPI0016")))
> +                       return "Host Bridge";

Might be good to add detection for the "cxl_test" bus here, currently
that shows "unknown device".

> +               if (!strcmp(property, "host"))
> +                       return "Switch Port";
> +               if (!strcmp(property, "memdev"))
> +                       return "Type 3 Memory Device";
> +               if (!strcmp(property, "dport"))
> +                       return "dport";
> +               if (!strcmp(property, "decoder"))
> +                       return "decoder";
> +       }
> +       return "unknown device";
> +}
> +
> +static bool check_device_type(struct json_object *device, char *type)
> +{
> +       return !strcmp(find_device_type(device), type);
> +}
> +
> +/* for labeling purposes */
> +static const char *find_device_ID(struct json_object *device)
> +{
> +       char *dev_type = find_device_type(device);
> +       json_object *ID = json_object_new_string("unknown");
> +
> +       if (!strcmp(dev_type, "ACPI0017 Device")) {
> +               json_object_put(ID);
> +               json_object_object_get_ex(device, "bus", &ID);
> +       }
> +
> +       if (!strcmp(dev_type, "Host Bridge") || !strcmp(dev_type, "Switch Port")) {
> +               json_object_put(ID);
> +               json_object_object_get_ex(device, "host", &ID);
> +       }
> +
> +       if (!strcmp(dev_type, "Type 3 Memory Device")) {
> +               json_object_put(ID);
> +               json_object_object_get_ex(device, "memdev", &ID);
> +       }
> +
> +       if (!strcmp(dev_type, "dport")) {
> +               json_object_put(ID);
> +               json_object_object_get_ex(device, "dport", &ID);
> +       }
> +
> +       return json_object_get_string(ID);
> +}
> +
> +static bool is_device(struct json_object *device)
> +{
> +       char *dev_type = find_device_type(device);
> +
> +       return (strcmp(dev_type, "dport") &&
> +               strcmp(dev_type, "decoder"));
> +}
> +
> +static char *label_device(struct json_object *device)
> +{
> +       char *label;
> +       const char *ID = find_device_ID(device);
> +       const char *devname = find_device_type(device);
> +
> +       asprintf(&label, "%s\nID: %s", devname, ID);
> +       if (!label)
> +               printf("Error: label allocation failed in %s\n",
> +                       __func__);
> +       return label;
> +}
> +
> +/* for determining number of devices listed in a json array */
> +static size_t count_top_devices(struct json_object *top_array)
> +{
> +       size_t dev_counter = 0;
> +       size_t top_array_len = json_object_array_length(top_array);
> +
> +       for (size_t idx = 0; idx < top_array_len; idx++)
> +               if (is_device(json_object_array_get_idx(top_array, idx)))
> +                       dev_counter++;
> +       return dev_counter;
> +}
> +
> +static void create_root_ports(struct json_object *host_bridge, Agraph_t *graph,
> +                      Agnode_t *hb)
> +{
> +       json_object *rps, *rp, *id_json;
> +       char *id, *rp_label;
> +       Agnode_t *rp_node;
> +       size_t nr_dports, idx;
> +
> +       assert(check_device_type(host_bridge, "Host Bridge"));
> +
> +       if (!json_object_object_get_ex(host_bridge, "dports", &rps))
> +               return;
> +
> +       nr_dports = json_object_array_length(rps);
> +
> +       for (idx = 0; idx < nr_dports; idx++) {
> +               rp = json_object_array_get_idx(rps, idx);
> +               json_object_object_get_ex(rp, "dport", &id_json);
> +               id = (char *)json_object_get_string(id_json);
> +               asprintf(&rp_label, "Root Port\nID: %s", id);
> +               if (!rp_label)
> +                       printf("Error: label allocation failed when creating root port nodes\n");
> +               rp_node = agnode(graph, rp_label, 1);
> +               agedge(graph, hb, rp_node, 0, 1);
> +               free(rp_label);
> +       }
> +}
> +
> +static const char *find_root_port(struct json_object *device)
> +{
> +       json_object *rp;
> +
> +       if (json_object_object_get_ex(device, "root port", &rp))
> +               return json_object_get_string(rp);
> +       return NULL;
> +}
> +
> +static char *find_rp_label(struct json_object *device)
> +{
> +       char *rp_node_name;
> +       const char *id = find_root_port(device);
> +
> +       if (!id)
> +               return NULL;
> +       asprintf(&rp_node_name, "Root Port\nID: %s", id);
> +       if (!rp_node_name)
> +               printf("Error: asprintf failed in %s\n", __func__);
> +       return rp_node_name;
> +}
> +
> +static Agnode_t **draw_subtree(struct json_object *current_array,
> +                              Agraph_t *graph)
> +{
> +       size_t json_array_len, nr_top_devices, obj_idx;
> +       size_t idx, nr_sub_devs, nr_devs_connected;
> +       char *label, *rp_label;
> +       Agnode_t **top_devices, **sub_devs, *parent_node;
> +       bool is_hb;
> +       json_object *device, *subdev_arr, *subdev;
> +       json_object_iter subdev_iter;
> +
> +       json_array_len = json_object_array_length(current_array);
> +       nr_top_devices = count_top_devices(current_array);
> +
> +       if (!nr_top_devices)
> +               return NULL;
> +
> +       top_devices = malloc(nr_top_devices * sizeof(device));
> +       if (!top_devices)
> +               printf("Error: Unable to allocate memory for top_devices\n");
> +
> +       for (obj_idx = 0; obj_idx < json_array_len; obj_idx++) {
> +               device = json_object_array_get_idx(current_array,
> +                                                  obj_idx);
> +               if (!is_device(device))
> +                       continue;
> +
> +               label = label_device(device);
> +               top_devices[obj_idx] = agnode(graph, label, 1);
> +
> +               agsafeset(top_devices[obj_idx], "shape", "box", "");
> +
> +               is_hb = check_device_type(device, "Host Bridge");
> +
> +               /* Create root port nodes if device is a host bridge */
> +               if (is_hb)
> +                       create_root_ports(device, graph,
> +                                         top_devices[obj_idx]);
> +
> +               free(label);
> +
> +               json_object_object_foreachC(device, subdev_iter) {
> +                       subdev_arr = subdev_iter.val;
> +
> +                       if (!json_object_is_type(subdev_arr, json_type_array))
> +                               continue;
> +                       nr_sub_devs = count_top_devices(subdev_arr);
> +                       sub_devs = draw_subtree(subdev_arr, graph);
> +                       if (!sub_devs)
> +                               continue;
> +                       if (!is_hb) {
> +                               for (idx = 0; idx < nr_sub_devs; idx++)
> +                                       agedge(graph, top_devices[obj_idx],
> +                                              sub_devs[idx], 0, 1);
> +                               free(sub_devs);
> +                               continue;
> +                       }
> +
> +                       nr_devs_connected = 0;
> +
> +                       for (idx = 0;
> +                            idx < json_object_array_length(subdev_arr);
> +                            idx++) {
> +                               subdev = json_object_array_get_idx(subdev_arr, idx);
> +                               if (!is_device(subdev))
> +                                       continue;
> +
> +                               rp_label = find_rp_label(subdev);
> +                               if (!rp_label) {
> +                                       printf("Error: please use a cxl list output containing root port attributes\n");
> +                                       return NULL;
> +                               }
> +                               parent_node = agnode(graph, rp_label, 0);
> +                               agedge(graph, parent_node,
> +                                      sub_devs[nr_devs_connected], 0, 1);
> +                               free(rp_label);
> +                               nr_devs_connected++;
> +                       }
> +                       free(sub_devs);
> +               }
> +       }
> +       return top_devices;
> +}
> +
> +struct json_object *parse_json_text(const char *path)
> +{
> +       FILE *fp;
> +       char *json_as_string;
> +       size_t file_len;
> +       json_object *json;
> +
> +       fp = fopen(path, "r");
> +       if (!fp)
> +               printf("Error: could not read file\n");
> +       fseek(fp, 0, SEEK_END);
> +       file_len = ftell(fp);
> +       fseek(fp, 0, SEEK_SET);
> +       json_as_string = malloc(file_len + 1);
> +       if (!json_as_string ||
> +           fread(json_as_string, 1, file_len, fp) != file_len) {
> +               free(json_as_string);
> +               printf("Error: could not read file %s\n", path);
> +       }
> +       json_as_string[file_len] = '\0';
> +       json = json_tokener_parse(json_as_string);
> +       return json;
> +}
> +
> +void create_image(const char *filename, json_object *platform)
> +{
> +       char *output_file = filename ? (char *)filename : "topology.jpg";
> +       GVC_t *gvc = gvContext();
> +       Agraph_t *graph = agopen("graph", Agdirected, 0);
> +       char *of_extension = strrchr(output_file, '.') + 1;
> +       Agnode_t **top_devices;
> +
> +       if (!of_extension || (strcmp(of_extension, "png") &&
> +                             strcmp(of_extension, "jpeg") &&
> +                             strcmp(of_extension, "jpg"))) {
> +               printf("Error: unacceptable output file type, please use .png, .jpeg, or .jpg");
> +               return;
> +       }
> +       top_devices = draw_subtree(platform, graph);
> +       free(top_devices);
> +       gvLayout(gvc, graph, "dot");
> +       /* Legality of the file extension is checked in list.c */
> +       gvRender(gvc, graph, strrchr(output_file, '.') + 1,
> +                fopen(output_file, "w"));
> +       gvFreeLayout(gvc, graph);
> +       agclose(graph);
> +}
> +

It might be nice to split all the graphviz interfacing helpers into a
separate graph.c file.

>  int cxl_filter_walk(struct cxl_ctx *ctx, struct cxl_filter_params *p)
>  {
>         struct json_object *jdevs = NULL, *jbuses = NULL, *jports = NULL;
> @@ -1209,7 +1483,10 @@ walk_children:
>                      top_level_objs > 1);
>         splice_array(p, jregions, jplatform, "regions", top_level_objs > 1);
>  
> -       util_display_json_array(stdout, jplatform, flags);
> +       if (p->image)
> +               create_image(p->output_file, jplatform);
> +       else
> +               util_display_json_array(stdout, jplatform, flags);

It might be nice to have the stdout output present unconditionally -
just duplicate it in the output file if requested, either as image or
text, but otherwise keep the usual stdout.

>  
>         return 0;
>  err:
> diff --git a/cxl/filter.h b/cxl/filter.h
> index 256df49c3d0c..aec7207aa885 100644
> --- a/cxl/filter.h
> +++ b/cxl/filter.h
> @@ -5,6 +5,8 @@
>  
>  #include <stdbool.h>
>  #include <util/log.h>
> +#include <json-c/json.h>
> +#include <graphviz/gvc.h>
>  
>  struct cxl_filter_params {
>         const char *memdev_filter;
> @@ -14,6 +16,8 @@ struct cxl_filter_params {
>         const char *endpoint_filter;
>         const char *decoder_filter;
>         const char *region_filter;
> +       const char *image_input_file;
> +       const char *output_file;
>         bool single;
>         bool endpoints;
>         bool decoders;
> @@ -26,6 +30,7 @@ struct cxl_filter_params {
>         bool human;
>         bool health;
>         bool partition;
> +       bool image;
>         int verbose;
>         struct log_ctx ctx;
>  };
> @@ -38,6 +43,8 @@ struct cxl_port *util_cxl_port_filter_by_memdev(struct cxl_port *port,
>                                                 const char *serial);
>  struct cxl_decoder *util_cxl_decoder_filter(struct cxl_decoder *decoder,
>                                             const char *__ident);
> +struct json_object *parse_json_text(const char *path);
> +void create_image(const char *filename, json_object *platform);
>  struct cxl_region *util_cxl_region_filter(struct cxl_region *region,
>                                           const char *__ident);
>  
> diff --git a/cxl/list.c b/cxl/list.c
> index 8c48fbbaaec3..3cbc0de26b2b 100644
> --- a/cxl/list.c
> +++ b/cxl/list.c
> @@ -52,6 +52,13 @@ static const struct option options[] = {
>                     "include memory device health information"),
>         OPT_BOOLEAN('I', "partition", &param.partition,
>                     "include memory device partition information"),
> +       OPT_BOOLEAN(0, "image", &param.image,
> +                  "display CXL topology"),
> +       OPT_STRING(0, "image-from-file", &param.image_input_file,
> +                  "input file path for image",
> +                  "path to file containing a json array describing the topology"),
> +       OPT_STRING('o', "output-file", &param.output_file, "output file path",
> +                  "path to file to output diagram to"),
>         OPT_INCR('v', "verbose", &param.verbose,
>                  "increase output detail"),
>  #ifdef ENABLE_DEBUG
> @@ -74,6 +81,9 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
>         };
>         int i;
>  
> +       json_object *platform;
> +       const char *output_file_name;
> +
>         argc = parse_options(argc, argv, options, u, 0);
>         for (i = 0; i < argc; i++)
>                 error("unknown parameter \"%s\"\n", argv[i]);
> @@ -86,6 +96,20 @@ int cmd_list(int argc, const char **argv, struct cxl_ctx *ctx)
>                 usage_with_options(u, options);
>         }
>  
> +       if (param.output_file)
> +               output_file_name = param.output_file;
> +
> +       if (param.image_input_file) {
> +               if (access(param.image_input_file, R_OK)) {
> +                       error("input file path is incorrect\n");
> +                       return 0;
> +               }
> +               platform = parse_json_text(param.image_input_file);
> +               create_image(output_file_name, platform);
> +               return 0;
> +       }
> +
> +
>         if (num_list_flags() == 0) {
>                 if (param.memdev_filter || param.serial_filter)
>                         param.memdevs = true;
> diff --git a/cxl/meson.build b/cxl/meson.build
> index f2474aaa6e2e..2670d1e6e330 100644
> --- a/cxl/meson.build
> +++ b/cxl/meson.build
> @@ -19,6 +19,7 @@ cxl_tool = executable('cxl',
>      kmod,
>      json,
>      versiondep,
> +    graphviz,
>    ],
>    install : true,
>    install_dir : rootbindir,
> diff --git a/meson.build b/meson.build
> index aecf461fc9ef..3c4f4fc006c3 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -141,6 +141,7 @@ kmod = dependency('libkmod')
>  libudev = dependency('libudev')
>  uuid = dependency('uuid')
>  json = dependency('json-c')
> +graphviz = dependency('libgvc')
>  if get_option('docs').enabled()
>    if get_option('asciidoctor').enabled()
>      asciidoc = find_program('asciidoctor', required : true)