[ovs-dev] [PATCH 3/3] command-line: add ovs_cmdl_context

[Ben Pfaff]

On Mon, Mar 16, 2015 at 12:01:56PM -0400, Russell Bryant wrote:
> I started working on a new command line utility that used this shared
> code.  I wanted the ability to pass some data from common
> initialization code to all of the commands.  You can find a similar
> pattern in ovs-vsctl.
> 
> This patch updates the command handler to take a new struct,
> ovs_cmdl_context, instead of argc and argv directly.  It includes argc
> and argv, but also includes an opaque type (void *), where the user of
> this API can attach its custom data it wants passed along to command
> handlers.
> 
> This patch affected the ovstest sub-programs, as well.  The patch
> includes a bit of an odd hack to OVSTEST_REGISTER() to avoid making
> the main() function of the sub-programs take a ovs_cmdl_context.
> The test main() functions still receive argc and argv directly, as
> that seems more natural.  The test-subprograms themselves are able to
> make use of a context internally, though.
> 
> Signed-off-by: Russell Bryant <rbryant at redhat.com>

Thanks for the patch!

I'm happy with this idea, although in writing it myself I would have a
hard time deciding whether to include the void * pointer or to expect
the caller to embed the structure into a larger one that could contain
additional data.

In ovstest.h I'd prefer to use a __ suffix rather than prefix, since
underscore prefixes are reserved to the C implementation.

Clang complains about a number of the initializers:

    ../utilities/ovs-benchmark.c:84:40: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
        struct ovs_cmdl_context ctx = { 0, };
                                           ^
    ../ovsdb/ovsdb-tool.c:58:40: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
        struct ovs_cmdl_context ctx = { 0, };
                                           ^
    1 error generated.
    make[2]: *** [utilities/ovs-benchmark.o] Error 1
    1../tests/test-ovsdb.c:61:40: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
        struct ovs_cmdl_context ctx = { 0, };
                                           ^
     error generated.
    make[2]: *** [ovsdb/ovsdb-tool.o] Error 1
    make[2]: *** [tests/test-bitmap.o] Error 1
    ../tests/test-jsonrpc.c:43:40: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
        struct ovs_cmdl_context ctx = { 0, };
                                           ^
    ../tests/test-ovsdb.c:1545:53: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
            struct ovs_cmdl_context transact_ctx = { 0, };
                                                        ^
    1 error generated.
    make[2]: *** [tests/test-jsonrpc.o] Error 1
    2 errors generated.
    make[2]: *** [tests/test-ovsdb.o] Error 1
    ../utilities/ovs-ofctl.c:120:40: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
        struct ovs_cmdl_context ctx = { 0, };
                                           ^
    ../tests/test-util.c:1107:40: error: missing field 'argv' initializer
          [-Werror,-Wmissing-field-initializers]
        struct ovs_cmdl_context ctx = { 0, };
                                           ^
    1 error generated.

Clang doesn't complain about { .argc = 0 } as an initializer.

Clang and GCC both complain about an incompatible function:

    ../tests/test-bitmap.c:153:5: error: initialization from incompatible pointer type [-Werror]
    ../tests/test-bitmap.c:153:5: error: (near initialization for 'commands[1].handler') [-Werror]

and

    ../tests/test-bitmap.c:153:31: error: incompatible pointer types initializing
          'ovs_cmdl_handler' (aka 'void (*)(struct ovs_cmdl_context *)') with an
          expression of type 'void (int, char **)'
          [-Werror,-Wincompatible-pointer-types]
        {"benchmark", NULL, 1, 1, run_benchmarks},
                                  ^~~~~~~~~~~~~~
    1 error generated.

Would you mind fixing that up?