RE: [EXT] Re: [PATCH v8 01/12] app/graph: add application framework to read CLI

[Sunil Kumar Kori <skori@marvell.com>]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Monday, October 16, 2023 2:31 PM
> To: Sunil Kumar Kori <skori@marvell.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Rakesh Kudurumalla
> <rkudurumalla@marvell.com>; dev@dpdk.org
> Subject: [EXT] Re: [PATCH v8 01/12] app/graph: add application framework
> to read CLI
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Fri, Sep 29, 2023 at 3:28 PM <skori@marvell.com> wrote:
> >
> > From: Sunil Kumar Kori <skori@marvell.com>
> >
> > It adds base framework to read a given .cli file as a command line
> > parameter "-s".
> >
> > Example:
> >  # ./dpdk-graph -c 0xff -- -s ./app/graph/examples/dummy.cli
> >
> > Each .cli file will contain commands to configure different module
> > like mempool, ethdev, lookup tables, graph etc. Command parsing is
> > backed by commandline library.
> >
> > Each module needs to expose its supported commands & corresponding
> > callback functions to commandline library to get them parsed.
> >
> > Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> > Signed-off-by: Rakesh Kudurumalla <rkudurumalla@marvell.com>
> 
> > +cli_script_process(const char *file_name, size_t msg_in_len_max,
> > +size_t msg_out_len_max, void *obj) {
> > +       char *msg_in = NULL, *msg_out = NULL;
> > +       FILE *f = NULL;
> > +
> > +       /* Check input arguments */
> > +       if ((file_name == NULL) || (strlen(file_name) == 0) || (msg_in_len_max
> == 0) ||
> > +           (msg_out_len_max == 0))
> > +               return -EINVAL;
> > +
> > +       msg_in = malloc(msg_in_len_max + 1);
> > +       msg_out = malloc(msg_out_len_max + 1);
> > +       if ((msg_in == NULL) || (msg_out == NULL)) {
> 
> Use goto to avod dupliciting this section.
> 
Will fix in next version.

> > +               free(msg_out);
> > +               free(msg_in);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       /* Open input file */
> > +       f = fopen(file_name, "r");
> > +       if (f == NULL) {
> > +               free(msg_out);
> > +               free(msg_in);
> > +               return -EIO;
> > +       }
> > +
> > +       /* Read file */
> > +       while (fgets(msg_in, msg_in_len_max, f) != NULL) {
> > +               msg_out[0] = 0;
> > +
> > +               cli_process(msg_in, msg_out, msg_out_len_max, obj);
> > +
> > +               if (strlen(msg_out))
> > +                       printf("%s", msg_out);
> > +       }
> > +
> > +       /* Close file */
> > +       fclose(f);
> 
> See above:
> 
Will fix in next version.

> > +       free(msg_out);
> > +       free(msg_in);
> > +       return 0;
> > +}
> >
> new file mode 100644
Will fix in next version.

> > index 0000000000..08d0a48cd9
> > --- /dev/null
> > +++ b/app/graph/meson.build
> > @@ -0,0 +1,15 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2023 Marvell.
> > +
> > +# override default name to drop the hyphen name = 'graph'
> > +build = cc.has_header('sys/epoll.h')
> 
> Is this required?
> 
Yes. It is needed to compiled  the application for FreeBSD.

> > +if not build
> > +    subdir_done()
> > +endif
> > +
> > +deps += ['bus_pci', 'graph', 'eal', 'lpm', 'ethdev', 'node',
> > +'cmdline']
> 
> Check bus_pci really required?
> 
Will check.

> > +sources = files(
> > +        'cli.c',
> > +        'main.c',
> > +)
> > diff --git a/app/graph/module_api.h b/app/graph/module_api.h new file
> > mode 100644 index 0000000000..372aeae7e3
> > --- /dev/null
> > +++ b/app/graph/module_api.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2023 Marvell.
> > + */
> > +
> > +#ifndef APP_GRAPH_MODULE_API_H
> > +#define APP_GRAPH_MODULE_API_H
> > +
> > +#include <stdint.h>
> > +#include <stdbool.h>
> 
> Have a new line here.
> 
Will fix in next version.

> > +#include "cli.h"
> > +/*
> > + * Externs
> > + */
> > new file mode 100644
> > index 0000000000..271a85896d
> > --- /dev/null
> > +++ b/doc/guides/tools/graph.rst
> > @@ -0,0 +1,82 @@
> > +..  SPDX-License-Identifier: BSD-3-Clause
> > +    Copyright(c) 2023 Marvell.
> > +
> > +dpdk-graph Application
> > +======================
> > +
> > +The ``dpdk-graph`` tool is a Data Plane Development Kit (DPDK)
> > +application that allows exercising various graph use cases.
> > +This application has a generic framework to add new graph based use
> > +cases to verify functionality. Each use case is defined as a ``<usecase>.cli``
> file.
> > +Based on the input file, application creates a graph to cater the use case.
> 
> Could you add some detailed text to describe the fact that, due to modular
> nature this app framework can be used by other graph based application OR
> SO.
> 
> > +
> > +Supported Use cases
> > +-------------------
> > + *
> 
> This * can be removed in this patch. Add it when l3fwd adds it.
> 
Will fix in next version.

> 
> > +
> > +Running the Application
> > +-----------------------
> > +
> > +The application has a number of command line options which can be
> > +provided in following syntax
> > +
> > +.. code-block:: console
> > +
> > +   dpdk-graph [EAL Options] -- [application options]
> > +
> > +EAL Options
> > +~~~~~~~~~~~
> > +
> > +Following are the EAL command-line options that can be used in
> > +conjunction with the ``dpdk-graph`` application.
> > +See the DPDK Getting Started Guides for more information on these
> options.
> > +
> > +*   ``-c <COREMASK>`` or ``-l <CORELIST>``
> > +
> > +        Set the hexadecimal bit mask of the cores to run on. The CORELIST is
> a
> > +        list of cores to be used.
> > +
> > +Application Options
> > +~~~~~~~~~~~~~~~~~~~
> > +
> > +Following are the application command-line options:
> > +
> > +* ``-s``
> > +
> > +        Script name with absolute path which specifies the use case. It is
> > +        a mandatory parameter which will be used to create desired graph
> > +        for a given use case.
> > +
> > +* ``--help``
> > +
> > +       Dumps application usage
> > +
> > +Supported CLI commands
> > +----------------------
> > +
> > +This section provides details on commands which can be used in
> > +``<usecase>.cli`` file to express the requested use case configuration.
> > +
> > +.. list-table:: Exposed CLIs
> > +   :widths: 40 40 10 10
> > +   :header-rows: 1
> > +   :class: longtable
> > +
> > +   * - Command
> > +     - Description
> > +     - Dynamic
> > +     - Optional
> > +   * - Dummy command
> > +     - Dummy command description
> > +     - No
> > +     - No
> 
> Is it possible to make it as more vertical placement ? More horizontal
> placement is not PDF friendly.
Will check it.