On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:
> From: Jakob Meng <c...@jakobmeng.de>
>
> For monitoring systems such as Prometheus it would be beneficial if
> OVS would expose statistics in a machine-readable format.
>
> This patch introduces support for different output formats to ovs-xxx
> tools. They gain a global option '-f,--format' which allows users to
> request JSON instead of plain-text for humans. An example call
> implemented in a later patch is 'ovs-appctl --format json dpif/show'.
>
> For that a new 'set-options' command has been added to lib/unixctl.c
> which allows to change the output format of the following commands.
> It is supposed to be run by ovs-xxx tools transparently for the user
> when a specific output format has been requested. For example, when a
> user calls 'ovs-appctl --format json dpif/show', then ovs-appctl will
> call 'set-options' to set the output format as requested by the user
> and then the actual command 'dpif/show'.
> This ovs-appctl behaviour has been implemented in a backward compatible
> way. One can use an updated client (ovs-appctl) with an old server
> (ovs-vswitchd) and vice versa. Of course, JSON output only works when
> both sides have been updated.
>
> To demonstrate the necessary API changes for supporting output formats,
> unixctl_command_register() in lib/unixctl.* has been cloned to
> unixctl_command_register_fmt(). The latter will be replaced with the
> former in a later patch. The new function gained an int argument called
> 'output_fmts' with which commands have to announce their support for
> output formats.
>
> Function type unixctl_cb_func in lib/unixctl.h has gained a new arg
> 'enum ovs_output_fmt fmt'. For now, it has been added as a comment
> only for the huge changes reason mentioned earlier. The output format
> which has been passed via 'set-options' to ovs-vswitchd will be
> converted into a value of type 'enum ovs_output_fmt' in lib/unixctl.c
> and then passed to said 'fmt' arg of the choosen command handler (in a
> future patch). When a requested output format is not supported by a
> command, then process_command() in lib/unixctl.c will return an error.
>
> This patch does not yet pass the choosen output format to commands.
> Doing so requires changes to all unixctl_command_register() calls and
> all command callbacks. To improve readibility those changes have been
> split out into a follow up patch. Respectively, whenever an output
> format other than 'text' is choosen for ovs-xxx tools, they will fail.
> By default, the output format is plain-text as before.
>
> In popular tools like kubectl the option for output control is usually
> called '-o|--output' instead of '-f,--format'. But ovs-appctl already
> has an short option '-o' which prints the available ovs-appctl options
> ('--option'). The now choosen name also better alines with ovsdb-client
> where '-f,--format' controls output formatting.
>
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <c...@jakobmeng.de>
Hi Jakob,
Thank you for submitting this series; I believe it's a valuable addition to
OVS! Apologies for the delayed response. I've reviewed the entire series, and
most of the comments are minor change requests. I'll hold off on sending a new
revision until Ilya has had a chance to review it, as he provided some comments
on the previous revision.
Cheers,
Eelco
> ---
> NEWS | 2 +
> lib/command-line.c | 36 +++++++++++
> lib/command-line.h | 10 +++
> lib/dpctl.h | 4 ++
> lib/unixctl.c | 142 ++++++++++++++++++++++++++++++++++-------
> lib/unixctl.h | 16 ++++-
> utilities/ovs-appctl.c | 97 ++++++++++++++++++++++++----
> utilities/ovs-dpctl.c | 1 +
> 8 files changed, 271 insertions(+), 37 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 2153b4805..8631ea45e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -33,6 +33,8 @@ v3.3.0 - xx xxx xxxx
> "ovs-appctl dpctl/ct-del-limits default".
> * 'dpctl/flush-conntrack' is now capable of flushing connections based
> on mark and labels.
> + * Added new option [-f|--format] to choose the output format, e.g.
> 'json'
> + or 'text' (by default).
> - ovs-vsctl:
> * New commands 'set-zone-limit', 'del-zone-limit' and 'list-zone-limits'
> to manage the maximum number of connections in conntrack zones via
> diff --git a/lib/command-line.c b/lib/command-line.c
> index 967f4f5d5..775e0430a 100644
> --- a/lib/command-line.c
> +++ b/lib/command-line.c
> @@ -24,6 +24,7 @@
> #include "ovs-thread.h"
> #include "util.h"
> #include "openvswitch/vlog.h"
> +#include "openvswitch/json.h"
>
> VLOG_DEFINE_THIS_MODULE(command_line);
>
> @@ -53,6 +54,41 @@ ovs_cmdl_long_options_to_short_options(const struct option
> options[])
> return xstrdup(short_options);
> }
>
> +const char *
> +ovs_output_fmt_to_string(enum ovs_output_fmt fmt)
> +{
> + switch (fmt) {
> + case OVS_OUTPUT_FMT_TEXT:
> + return "text";
> +
> + case OVS_OUTPUT_FMT_JSON:
> + return "json";
> +
> + default:
> + return NULL;
> + }
> +}
> +
> +struct json *
> +ovs_output_fmt_to_json(enum ovs_output_fmt fmt)
> +{
> + const char *string = ovs_output_fmt_to_string(fmt);
> + return string ? json_string_create(string) : NULL;
> +}
> +
> +bool
> +ovs_output_fmt_from_string(const char *string, enum ovs_output_fmt *fmt)
> +{
> + if (!strcmp(string, "text")) {
> + *fmt = OVS_OUTPUT_FMT_TEXT;
> + } else if (!strcmp(string, "json")) {
> + *fmt = OVS_OUTPUT_FMT_JSON;
> + } else {
> + return false;
> + }
> + return true;
> +}
> +
> static char * OVS_WARN_UNUSED_RESULT
> build_short_options(const struct option *long_options)
> {
> diff --git a/lib/command-line.h b/lib/command-line.h
> index fc2a2690f..494274cec 100644
> --- a/lib/command-line.h
> +++ b/lib/command-line.h
> @@ -20,6 +20,7 @@
> /* Utilities for command-line parsing. */
>
> #include "compiler.h"
> +#include <openvswitch/list.h>
>
> struct option;
>
> @@ -46,6 +47,15 @@ struct ovs_cmdl_command {
>
> char *ovs_cmdl_long_options_to_short_options(const struct option *options);
>
> +enum ovs_output_fmt {
> + OVS_OUTPUT_FMT_TEXT = 1 << 0,
> + OVS_OUTPUT_FMT_JSON = 1 << 1
> +};
> +
> +const char *ovs_output_fmt_to_string(enum ovs_output_fmt);
> +struct json *ovs_output_fmt_to_json(enum ovs_output_fmt);
> +bool ovs_output_fmt_from_string(const char *, enum ovs_output_fmt *);
> +
> struct ovs_cmdl_parsed_option {
> const struct option *o;
> char *arg;
> diff --git a/lib/dpctl.h b/lib/dpctl.h
> index 9d0052152..1c9b2409f 100644
> --- a/lib/dpctl.h
> +++ b/lib/dpctl.h
> @@ -19,6 +19,7 @@
> #include <stdbool.h>
>
> #include "compiler.h"
> +#include "command-line.h"
>
> struct dpctl_params {
> /* True if it is called by ovs-appctl command. */
> @@ -42,6 +43,9 @@ struct dpctl_params {
> /* --names: Use port names in output? */
> bool names;
>
> + /* Output format. */
> + enum ovs_output_fmt format;
> +
> /* Callback for printing. This function is called from
> dpctl_run_command()
> * to output data. The 'aux' parameter is set to the 'aux'
> * member. The 'error' parameter is true if 'string' is an error
> diff --git a/lib/unixctl.c b/lib/unixctl.c
> index 103357ee9..dd1450f22 100644
> --- a/lib/unixctl.c
> +++ b/lib/unixctl.c
> @@ -21,7 +21,6 @@
> #include "coverage.h"
> #include "dirs.h"
> #include "openvswitch/dynamic-string.h"
> -#include "openvswitch/json.h"
> #include "jsonrpc.h"
> #include "openvswitch/list.h"
> #include "openvswitch/poll-loop.h"
> @@ -39,6 +38,7 @@ COVERAGE_DEFINE(unixctl_replied);
> struct unixctl_command {
> const char *usage;
> int min_args, max_args;
> + int output_fmts;
Considering that this involves combining bit-mask enums, I suggest converting
this to an unsigned int.
> unixctl_cb_func *cb;
> void *aux;
> };
> @@ -50,6 +50,8 @@ struct unixctl_conn {
> /* Only one request can be in progress at a time. While the request is
> * being processed, 'request_id' is populated, otherwise it is null. */
> struct json *request_id; /* ID of the currently active request. */
> +
> + enum ovs_output_fmt fmt;
> };
>
> /* Server for control connection. */
> @@ -94,6 +96,39 @@ unixctl_version(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> unixctl_command_reply(conn, ovs_get_program_version());
> }
>
> +static void
> +unixctl_set_options(struct unixctl_conn *conn, int argc, const char *argv[],
> + void *aux OVS_UNUSED)
> +{
> + char * error = NULL;
> +
> + for (size_t i = 1; i < argc; i++) {
> + if ((i + 1) == argc) {
> + error = xasprintf("option requires an argument -- %s", argv[i]);
> + goto error;
> + } else if (!strcmp(argv[i], "--format")) {
> + /* Move index to option argument. */
> + i++;
> +
> + /* Parse output format argument. */
> + if (!ovs_output_fmt_from_string(argv[i], &conn->fmt)) {
> + error = xasprintf("option %s has invalid value %s",
> + argv[i - 1], argv[i]);
> + goto error;
> + }
> + } else {
> + error = xasprintf("invalid option -- %s", argv[i]);
> + goto error;
> +
> + }
> + }
> + unixctl_command_reply(conn, NULL);
> + return;
> +error:
> + unixctl_command_reply_error(conn, error);
> + free(error);
> +}
> +
> /* Registers a unixctl command with the given 'name'. 'usage' describes the
> * arguments to the command; it is used only for presentation to the user in
> * "list-commands" output. (If 'usage' is NULL, then the command is hidden.)
> @@ -109,6 +144,28 @@ void
> unixctl_command_register(const char *name, const char *usage,
> int min_args, int max_args,
> unixctl_cb_func *cb, void *aux)
> +{
> + unixctl_command_register_fmt(name, usage, min_args, max_args,
> + OVS_OUTPUT_FMT_TEXT, cb, aux);
> +}
> +
> +/* Registers a unixctl command with the given 'name'. 'usage' describes the
> + * arguments to the command; it is used only for presentation to the user in
> + * "list-commands" output. (If 'usage' is NULL, then the command is hidden.)
> + * 'output_fmts' is a bitmap that defines what output formats a command
> + * supports, e.g. OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON.
> + *
> + * 'cb' is called when the command is received. It is passed an array
> + * containing the command name and arguments, plus a copy of 'aux'. Normally
> + * 'cb' should reply by calling unixctl_command_reply() or
> + * unixctl_command_reply_error() before it returns, but if the command cannot
> + * be handled immediately then it can defer the reply until later. A given
> + * connection can only process a single request at a time, so a reply must be
> + * made eventually to avoid blocking that connection. */
> +void
> +unixctl_command_register_fmt(const char *name, const char *usage,
> + int min_args, int max_args, int output_fmts,
Unsigned int for output_fmts?
> + unixctl_cb_func *cb, void *aux)
> {
> struct unixctl_command *command;
> struct unixctl_command *lookup = shash_find_data(&commands, name);
> @@ -123,41 +180,48 @@ unixctl_command_register(const char *name, const char
> *usage,
> command->usage = usage;
> command->min_args = min_args;
> command->max_args = max_args;
> + command->output_fmts = output_fmts;
> command->cb = cb;
> command->aux = aux;
> shash_add(&commands, name, command);
> }
>
> -static void
> -unixctl_command_reply__(struct unixctl_conn *conn,
> - bool success, const char *body)
> +static struct json *
> +json_string_create__(const char *body)
> {
> - struct json *body_json;
> - struct jsonrpc_msg *reply;
> -
> - COVERAGE_INC(unixctl_replied);
> - ovs_assert(conn->request_id);
> -
> if (!body) {
> body = "";
> }
>
> if (body[0] && body[strlen(body) - 1] != '\n') {
> - body_json = json_string_create_nocopy(xasprintf("%s\n", body));
> + return json_string_create_nocopy(xasprintf("%s\n", body));
> } else {
> - body_json = json_string_create(body);
> + return json_string_create(body);
We can remove the else clause here, so it will become:
static struct json *
json_string_create__(const char *body)
{
if (!body) {
body = "";
}
if (body[0] && body[strlen(body) - 1] != '\n') {
return json_string_create_nocopy(xasprintf("%s\n", body));
}
return json_string_create(body);
}
> }
> +}
> +
> +/* Takes ownership of 'body'. */
> +static void
> +unixctl_command_reply__(struct unixctl_conn *conn,
> + bool success, struct json *body)
> +{
> + struct jsonrpc_msg *reply;
> +
> + COVERAGE_INC(unixctl_replied);
> + ovs_assert(conn->request_id);
>
> if (success) {
> - reply = jsonrpc_create_reply(body_json, conn->request_id);
> + reply = jsonrpc_create_reply(body, conn->request_id);
> } else {
> - reply = jsonrpc_create_error(body_json, conn->request_id);
> + reply = jsonrpc_create_error(body, conn->request_id);
> }
>
> if (VLOG_IS_DBG_ENABLED()) {
> char *id = json_to_string(conn->request_id, 0);
> + char *msg = json_to_string(body, 0);
> VLOG_DBG("replying with %s, id=%s: \"%s\"",
> - success ? "success" : "error", id, body);
> + success ? "success" : "error", id, msg);
> + free(msg);
> free(id);
> }
>
> @@ -170,22 +234,34 @@ unixctl_command_reply__(struct unixctl_conn *conn,
>
> /* Replies to the active unixctl connection 'conn'. 'result' is sent to the
> * client indicating the command was processed successfully. Only one call
> to
> - * unixctl_command_reply() or unixctl_command_reply_error() may be made per
> - * request. */
> + * unixctl_command_reply(), unixctl_command_reply_error() or
> + * unixctl_command_reply_json() may be made per request. */
> void
> unixctl_command_reply(struct unixctl_conn *conn, const char *result)
> {
> - unixctl_command_reply__(conn, true, result);
> + unixctl_command_reply__(conn, true, json_string_create__(result));
> }
>
> /* Replies to the active unixctl connection 'conn'. 'error' is sent to the
> - * client indicating an error occurred processing the command. Only one
> call to
> - * unixctl_command_reply() or unixctl_command_reply_error() may be made per
> - * request. */
> + * client indicating an error occurred processing the command. Only one call
> + * to unixctl_command_reply(), unixctl_command_reply_error() or
> + * unixctl_command_reply_json() may be made per request. */
> void
> unixctl_command_reply_error(struct unixctl_conn *conn, const char *error)
> {
> - unixctl_command_reply__(conn, false, error);
> + unixctl_command_reply__(conn, false, json_string_create__(error));
> +}
> +
> +/* Replies to the active unixctl connection 'conn'. 'result' is sent to the
> + * client indicating the command was processed successfully. Only one call
> to
> + * unixctl_command_reply(), unixctl_command_reply_error() or
> + * unixctl_command_reply_json() may be made per request.
> + *
> + * Takes ownership of 'body'. */
> +void
> +unixctl_command_reply_json(struct unixctl_conn *conn, struct json *body)
> +{
> + unixctl_command_reply__(conn, true, body);
> }
>
> /* Creates a unixctl server listening on 'path', which for POSIX may be:
> @@ -250,6 +326,8 @@ unixctl_server_create(const char *path, struct
> unixctl_server **serverp)
> unixctl_command_register("list-commands", "", 0, 0,
> unixctl_list_commands,
> NULL);
> unixctl_command_register("version", "", 0, 0, unixctl_version, NULL);
> + unixctl_command_register("set-options", "[--format text|json]", 2, 2,
> + unixctl_set_options, NULL);
>
> struct unixctl_server *server = xmalloc(sizeof *server);
> server->listener = listener;
> @@ -291,6 +369,18 @@ process_command(struct unixctl_conn *conn, struct
> jsonrpc_msg *request)
> } else if (params->n > command->max_args) {
> error = xasprintf("\"%s\" command takes at most %d arguments",
> request->method, command->max_args);
> + } else if ((!command->output_fmts && conn->fmt != OVS_OUTPUT_FMT_TEXT) ||
> + (command->output_fmts && !(conn->fmt & command->output_fmts)))
> + {
> + error = xasprintf("\"%s\" command does not support output format"\
> + " \"%s\" (supported: %d, requested: %d)",
> + request->method,
> ovs_output_fmt_to_string(conn->fmt),
> + command->output_fmts, conn->fmt);
> + } else if (conn->fmt != OVS_OUTPUT_FMT_TEXT) {
> + /* FIXME: Remove this check once output format will be passed to the
> + * command handler below. */
> + error = xasprintf("output format \"%s\" has not been implemented
> yet",
> + ovs_output_fmt_to_string(conn->fmt));
> } else {
> struct svec argv = SVEC_EMPTY_INITIALIZER;
> int i;
> @@ -307,8 +397,10 @@ process_command(struct unixctl_conn *conn, struct
> jsonrpc_msg *request)
> svec_terminate(&argv);
>
> if (!error) {
> + /* FIXME: Output format will be passed as 'fmt' to the command in
> + * later patch. */
> command->cb(conn, argv.n, (const char **) argv.names,
> - command->aux);
> + /* conn->fmt, */ command->aux);
> }
>
> svec_destroy(&argv);
> @@ -381,6 +473,7 @@ unixctl_server_run(struct unixctl_server *server)
> struct unixctl_conn *conn = xzalloc(sizeof *conn);
> ovs_list_push_back(&server->conns, &conn->node);
> conn->rpc = jsonrpc_open(stream);
> + conn->fmt = OVS_OUTPUT_FMT_TEXT;
> } else if (error == EAGAIN) {
> break;
> } else {
> @@ -518,6 +611,9 @@ unixctl_client_transact(struct jsonrpc *client, const
> char *command, int argc,
> } else if (reply->result) {
> if (reply->result->type == JSON_STRING) {
> *result = xstrdup(json_string(reply->result));
> + } else if (reply->result->type == JSON_OBJECT ||
> + reply->result->type == JSON_ARRAY) {
> + *result = json_to_string(reply->result, 0);
> } else {
> VLOG_WARN("%s: unexpected result type in JSON rpc reply: %s",
> jsonrpc_get_name(client),
> diff --git a/lib/unixctl.h b/lib/unixctl.h
> index 4562dbc49..8200b9e11 100644
> --- a/lib/unixctl.h
> +++ b/lib/unixctl.h
> @@ -17,6 +17,9 @@
> #ifndef UNIXCTL_H
> #define UNIXCTL_H 1
>
> +#include "openvswitch/json.h"
> +#include "command-line.h"
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
> @@ -40,13 +43,24 @@ int unixctl_client_transact(struct jsonrpc *client,
>
> /* Command registration. */
> struct unixctl_conn;
> +/* FIXME: Output format will be passed as 'fmt' to the command in later
> patch.
> + */
> typedef void unixctl_cb_func(struct unixctl_conn *,
> - int argc, const char *argv[], void *aux);
> + int argc, const char *argv[],
> + /* enum ovs_output_fmt fmt, */ void *aux);
> +/* FIXME: unixctl_command_register() will be replaced with
> + * unixctl_command_register_fmt() in a later patch of this series. It
> + * is kept temporarily to reduce the amount of changes in this patch.
> */
> void unixctl_command_register(const char *name, const char *usage,
> int min_args, int max_args,
> unixctl_cb_func *cb, void *aux);
> +void unixctl_command_register_fmt(const char *name, const char *usage,
> + int min_args, int max_args, int
> output_fmts,
> + unixctl_cb_func *cb, void *aux);
> void unixctl_command_reply_error(struct unixctl_conn *, const char *error);
> void unixctl_command_reply(struct unixctl_conn *, const char *body);
> +void unixctl_command_reply_json(struct unixctl_conn *,
> + struct json *body);
>
> #ifdef __cplusplus
> }
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index ba0c172e6..02df8ba97 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -29,12 +29,22 @@
> #include "jsonrpc.h"
> #include "process.h"
> #include "timeval.h"
> +#include "svec.h"
> #include "unixctl.h"
> #include "util.h"
> #include "openvswitch/vlog.h"
>
> static void usage(void);
> -static const char *parse_command_line(int argc, char *argv[]);
> +
> +/* Parsed command line args. */
> +struct cmdl_args {
> + enum ovs_output_fmt format;
> + char *target;
> +};
> +
> +static struct cmdl_args *cmdl_args_create(void);
> +static void cmdl_args_destroy(struct cmdl_args *);
> +static struct cmdl_args *parse_command_line(int argc, char *argv[]);
> static struct jsonrpc *connect_to_target(const char *target);
>
> int
> @@ -43,30 +53,59 @@ main(int argc, char *argv[])
> char *cmd_result, *cmd_error;
> struct jsonrpc *client;
> char *cmd, **cmd_argv;
> - const char *target;
> + struct cmdl_args *args;
> int cmd_argc;
> int error;
> + struct svec opt_argv = SVEC_EMPTY_INITIALIZER;
Can we keep the variables in reversed Christmas tree order?
>
> set_program_name(argv[0]);
>
> /* Parse command line and connect to target. */
> - target = parse_command_line(argc, argv);
> - client = connect_to_target(target);
> + args = parse_command_line(argc, argv);
> + client = connect_to_target(args->target);
> +
> + /* Transact options request (if required) and process reply */
> + if (args->format != OVS_OUTPUT_FMT_TEXT) {
> + svec_add(&opt_argv, "--format");
> + svec_add(&opt_argv, ovs_output_fmt_to_string(args->format));
> + }
> + svec_terminate(&opt_argv);
> +
> + if (opt_argv.n > 0) {
You can use svec_is_empty() here.
> + error = unixctl_client_transact(client, "set-options",
> + opt_argv.n, opt_argv.names,
> + &cmd_result, &cmd_error);
> +
> + if (error) {
> + ovs_fatal(error, "%s: transaction error", args->target);
> + }
>
> - /* Transact request and process reply. */
> + if (cmd_error) {
> + jsonrpc_close(client);
> + fputs(cmd_error, stderr);
> + ovs_error(0, "%s: server returned an error", args->target);
> + exit(2);
> + }
> +
> + free(cmd_result);
> + free(cmd_error);
cmd_error will never end up here, as you call exit(2) above, so the free should
also move up.
Should we not exit on a transaction error also? I think error handling might
need some more cleanup.
> + }
> + svec_destroy(&opt_argv);
> +
> + /* Transact command request and process reply. */
> cmd = argv[optind++];
> cmd_argc = argc - optind;
> cmd_argv = cmd_argc ? argv + optind : NULL;
> error = unixctl_client_transact(client, cmd, cmd_argc, cmd_argv,
> &cmd_result, &cmd_error);
> if (error) {
> - ovs_fatal(error, "%s: transaction error", target);
> + ovs_fatal(error, "%s: transaction error", args->target);
> }
>
> if (cmd_error) {
> jsonrpc_close(client);
> fputs(cmd_error, stderr);
> - ovs_error(0, "%s: server returned an error", target);
> + ovs_error(0, "%s: server returned an error", args->target);
> exit(2);
> } else if (cmd_result) {
> fputs(cmd_result, stdout);
> @@ -74,6 +113,7 @@ main(int argc, char *argv[])
> OVS_NOT_REACHED();
> }
>
> + cmdl_args_destroy(args);
> jsonrpc_close(client);
> free(cmd_result);
> free(cmd_error);
> @@ -101,13 +141,34 @@ Common commands:\n\
> vlog/reopen Make the program reopen its log file\n\
> Other options:\n\
> --timeout=SECS wait at most SECS seconds for a response\n\
> + -f, --format=FMT Output format. One of: 'json', or 'text'\n\
> + ('text', by default)\n\
> -h, --help Print this helpful information\n\
> -V, --version Display ovs-appctl version information\n",
> program_name, program_name);
> exit(EXIT_SUCCESS);
> }
>
> -static const char *
> +static struct cmdl_args *
> +cmdl_args_create(void) {
> + struct cmdl_args *args = xmalloc(sizeof *args);
> +
> + args->format = OVS_OUTPUT_FMT_TEXT;
> + args->target = NULL;
> +
> + return args;
> +}
> +
> +static void
> +cmdl_args_destroy(struct cmdl_args *args) {
> + if (args->target) {
No need to check for NULL here, free() will do that for you.
> + free(args->target);
> + }
> +
> + free(args);
> +}
> +
> +static struct cmdl_args *
> parse_command_line(int argc, char *argv[])
> {
> enum {
> @@ -117,6 +178,7 @@ parse_command_line(int argc, char *argv[])
> static const struct option long_options[] = {
> {"target", required_argument, NULL, 't'},
> {"execute", no_argument, NULL, 'e'},
> + {"format", required_argument, NULL, 'f'},
> {"help", no_argument, NULL, 'h'},
> {"option", no_argument, NULL, 'o'},
> {"version", no_argument, NULL, 'V'},
> @@ -126,11 +188,11 @@ parse_command_line(int argc, char *argv[])
> };
> char *short_options_ =
> ovs_cmdl_long_options_to_short_options(long_options);
> char *short_options = xasprintf("+%s", short_options_);
> - const char *target;
> +
No need for extra newline.
> + struct cmdl_args *args = cmdl_args_create();
> int e_options;
> unsigned int timeout = 0;
While your add it maybe swap the above two lines.
>
> - target = NULL;
> e_options = 0;
> for (;;) {
> int option;
> @@ -141,10 +203,10 @@ parse_command_line(int argc, char *argv[])
> }
> switch (option) {
> case 't':
> - if (target) {
> + if (args->target) {
> ovs_fatal(0, "-t or --target may be specified only once");
> }
> - target = optarg;
> + args->target = xstrdup(optarg);
> break;
>
> case 'e':
> @@ -157,6 +219,12 @@ parse_command_line(int argc, char *argv[])
> }
> break;
>
> + case 'f':
> + if (!ovs_output_fmt_from_string(optarg, &args->format)) {
> + ovs_fatal(0, "value %s on -f or --format is invalid",
> optarg);
> + }
> + break;
> +
> case 'h':
> usage();
> break;
> @@ -194,7 +262,10 @@ parse_command_line(int argc, char *argv[])
> "(use --help for help)");
> }
>
> - return target ? target : "ovs-vswitchd";
> + if (!args->target) {
> + args->target = xstrdup("ovs-vswitchd");
> + }
> + return args;
> }
>
> static struct jsonrpc *
> diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
> index 56d7a942b..30104ddb2 100644
> --- a/utilities/ovs-dpctl.c
> +++ b/utilities/ovs-dpctl.c
> @@ -63,6 +63,7 @@ main(int argc, char *argv[])
> fatal_ignore_sigpipe();
>
> dpctl_p.is_appctl = false;
> + dpctl_p.format = OVS_OUTPUT_FMT_TEXT;
> dpctl_p.output = dpctl_print;
> dpctl_p.usage = usage;
>
> --
> 2.39.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
