On 7/4/24 16:09, [email protected] wrote: > From: Jakob Meng <[email protected]> > > 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-appctl. It gains a global option '-f,--format' which changes it to > print a JSON document instead of plain-text for humans. For example, a > later patch implements support for > 'ovs-appctl --format json dpif/show'. By default, the output format > is plain-text as before. > > A new 'set-options' command has been added to lib/unixctl.c which > allows to change the output format of the commands executed afterwards > on the same socket connection. It is supposed to be run by ovs-appctl > 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 afterwards it will call 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. > > Two access functions unixctl_command_{get,set}_output_format() and a > unixctl_command_reply_json function have been added to lib/unixctl.h: > unixctl_command_get_output_format() is supposed to be used in commands > like 'dpif/show' to query the requested output format. When JSON output > has been selected, the unixctl_command_reply_json() function can be > used to return JSON objects to the client (ovs-appctl) instead of > plain-text with the unixctl_command_reply{,_error}() functions. > > When JSON has been requested but a command has not implemented JSON > output the plain-text output will be wrapped in a provisional JSON > document with the following structure: > > {"reply":"$PLAIN_TEXT_HERE","reply-format":"plain"} > > Thus commands which have been executed successfully will not fail when > they try to render the output at a later stage. > > A test for the 'version' command has been implemented which shows how > the provisional JSON document looks like in practice. For a cleaner > JSON document, the trailing newline has been moved from the program > version string to function ovs_print_version(). This way, the > plain-text output of the 'version' command has not changed. > > Output formatting has been moved from unixctl_client_transact() in > lib/unixctl.c to utilities/ovs-appctl.c. The former merely returns the > JSON objects returned from the server and the latter is now responsible > for printing it properly. > > 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 alignes with
* chosen * aligns > ovsdb-client where '-f,--format' controls output formatting. > > Reported-at: https://bugzilla.redhat.com/1824861 > Signed-off-by: Jakob Meng <[email protected]> > --- > Documentation/ref/ovs-appctl.8.rst | 12 ++ > NEWS | 3 + > lib/unixctl.c | 183 ++++++++++++++++++++++------- > lib/unixctl.h | 17 ++- > lib/util.c | 6 +- > python/ovs/unixctl/server.py | 3 - > tests/appctl.py | 5 + > tests/ovs-vswitchd.at | 12 ++ > utilities/ovs-appctl.c | 135 ++++++++++++++++++--- > 9 files changed, 307 insertions(+), 69 deletions(-) > > diff --git a/Documentation/ref/ovs-appctl.8.rst > b/Documentation/ref/ovs-appctl.8.rst > index 3ce02e984..148cc7632 100644 > --- a/Documentation/ref/ovs-appctl.8.rst > +++ b/Documentation/ref/ovs-appctl.8.rst > @@ -8,6 +8,7 @@ Synopsis > ``ovs-appctl`` > [``--target=``<target> | ``-t`` <target>] > [``--timeout=``<secs> | ``-T`` <secs>] > +[``--format=``<format> | ``-f`` <format>] > <command> [<arg>...] > > ``ovs-appctl --help`` > @@ -67,6 +68,17 @@ In normal use only a single option is accepted: > runtime to approximately <secs> seconds. If the timeout expires, > ``ovs-appctl`` exits with a ``SIGALRM`` signal. > > +* ``-f <format>`` or ``--format=<format>`` > + > + Tells ``ovs-appctl`` which output format to use. By default, or with a > + <format> of ``text``, ``ovs-appctl`` will print plain-text for humans. > + When <format> is ``json``, ``ovs-appctl`` will return a JSON document. > + When ``json`` is requested, but a command has not implemented JSON > + output, the plain-text output will be wrapped in a provisional JSON > + document with the following structure:: > + > + {"reply-format":"plain","reply":"$PLAIN_TEXT_HERE"} > + > Common Commands > =============== > > diff --git a/NEWS b/NEWS > index e0359b759..f182647c7 100644 > --- a/NEWS > +++ b/NEWS > @@ -2,6 +2,9 @@ Post-v3.3.0 > -------------------- > - Option '--mlockall' now only locks memory pages on fault, if possible. > This also makes it compatible with vHost Post-copy Live Migration. > + - ovs-appctl: > + * Added new option [-f|--format] to choose the output format, e.g. > 'json' > + or 'text' (by default). > - Userspace datapath: > * Conntrack now supports 'random' flag for selecting ports in a range > while natting and 'persistent' flag for selection of the IP address > diff --git a/lib/unixctl.c b/lib/unixctl.c > index 103357ee9..66f5de163 100644 > --- a/lib/unixctl.c > +++ b/lib/unixctl.c > @@ -17,11 +17,13 @@ > #include <config.h> > #include "unixctl.h" > #include <errno.h> > +#include <getopt.h> > #include <unistd.h> > +#include "command-line.h" > #include "coverage.h" > #include "dirs.h" > #include "openvswitch/dynamic-string.h" > -#include "openvswitch/json.h" > +#include "json.h" The public header is fine here, no need to replace. > #include "jsonrpc.h" > #include "openvswitch/list.h" > #include "openvswitch/poll-loop.h" > @@ -50,6 +52,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 unixctl_output_fmt fmt; /* Output format of current connection. */ > }; > > /* Server for control connection. */ > @@ -63,6 +67,30 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, > 5); > > static struct shash commands = SHASH_INITIALIZER(&commands); > > +const char * > +unixctl_output_fmt_to_string(enum unixctl_output_fmt fmt) > +{ > + switch (fmt) { > + case UNIXCTL_OUTPUT_FMT_TEXT: return "text"; > + case UNIXCTL_OUTPUT_FMT_JSON: return "json"; > + default: return "<unknown>"; > + } > +} > + > +bool > +unixctl_output_fmt_from_string(const char *string, > + enum unixctl_output_fmt *fmt) > +{ > + if (!strcasecmp(string, "text")) { > + *fmt = UNIXCTL_OUTPUT_FMT_TEXT; > + } else if (!strcasecmp(string, "json")) { > + *fmt = UNIXCTL_OUTPUT_FMT_JSON; > + } else { > + return false; > + } > + return true; > +} > + > static void > unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED, > const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) > @@ -94,6 +122,53 @@ 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) > +{ > + struct ovs_cmdl_parsed_option *parsed_options = NULL; > + size_t n_parsed_options; > + char *error = NULL; > + > + static const struct option options[] = { > + {"format", required_argument, NULL, 'f'}, > + {NULL, 0, NULL, 0}, > + }; > + > + error = ovs_cmdl_parse_all(argc--, (char **) (argv++), options, > + &parsed_options, &n_parsed_options); > + if (error) { > + goto error; > + } > + > + for (size_t i = 0; i < n_parsed_options; i++) { > + struct ovs_cmdl_parsed_option *parsed_option = &parsed_options[i]; > + > + switch (parsed_option->o->val) { > + case 'f': > + /* format */ This comment seems unnecessary. > + if (!unixctl_output_fmt_from_string(parsed_option->arg, > + &conn->fmt)) { > + error = xasprintf("option format has invalid value %s", > + parsed_option->arg); > + goto error; > + } > + break; > + > + default: > + OVS_NOT_REACHED(); > + } > + } > + > + unixctl_command_reply(conn, NULL); > + free(parsed_options); > + return; > +error: > + unixctl_command_reply_error(conn, error); > + free(error); > + free(parsed_options); > +} > + > /* 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.) > @@ -128,36 +203,35 @@ unixctl_command_register(const char *name, const char > *usage, > shash_add(&commands, name, command); > } > > +enum unixctl_output_fmt > +unixctl_command_get_output_format(struct unixctl_conn *conn) > +{ > + return conn->fmt; > +} > + > +/* Takes ownership of the 'body'. */ > static void > unixctl_command_reply__(struct unixctl_conn *conn, > - bool success, const char *body) > + bool success, struct json *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)); > - } else { > - body_json = json_string_create(body); > - } > - > 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, JSSF_SORT); > + > VLOG_DBG("replying with %s, id=%s: \"%s\"", > - success ? "success" : "error", id, body); > + success ? "success" : "error", id, msg); > + free(msg); > free(id); > } > > @@ -169,23 +243,52 @@ 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. */ > + * client indicating the command was processed successfully. 'result' should > + * be plain-text; use unixctl_command_reply_json() to return a JSON document > + * when JSON output has been requested. Only one call to > + * unixctl_command_reply*() functions may be made per request. */ > void > unixctl_command_reply(struct unixctl_conn *conn, const char *result) > { > - unixctl_command_reply__(conn, true, result); > + struct json *json_result = json_string_create(result ? result : ""); > + > + if (conn->fmt == UNIXCTL_OUTPUT_FMT_JSON) { > + /* Wrap plain-text reply in provisional JSON document when JSON > output > + * has been requested. */ > + struct json *json_reply = json_object_create(); > + > + json_object_put_string(json_reply, "reply-format", "plain"); > + json_object_put(json_reply, "reply", json_result); > + > + json_result = json_reply; > + } > + > + unixctl_command_reply__(conn, true, json_result); > +} > + > +/* Replies to the active unixctl connection 'conn'. 'body' is sent to the > + * client indicating the command was processed successfully. Use this > function > + * when JSON output has been requested; otherwise use unixctl_command_reply() > + * for plain-text output. Only one call to unixctl_command_reply*() > functions > + * may be made per request. > + * > + * Takes ownership of the 'body'. */ > +void > +unixctl_command_reply_json(struct unixctl_conn *conn, struct json *body) > +{ > + ovs_assert(conn->fmt == UNIXCTL_OUTPUT_FMT_JSON); > + unixctl_command_reply__(conn, true, body); > } > > /* 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. 'error' > should > + * be plain-text. Only one call to unixctl_command_reply*() functions 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 ? error : "")); > } > > /* Creates a unixctl server listening on 'path', which for POSIX may be: > @@ -250,6 +353,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]", 1, 2, > + unixctl_set_options, NULL); > > struct unixctl_server *server = xmalloc(sizeof *server); > server->listener = listener; > @@ -381,6 +486,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 = UNIXCTL_OUTPUT_FMT_TEXT; > } else if (error == EAGAIN) { > break; > } else { > @@ -483,7 +589,7 @@ unixctl_client_create(const char *path, struct jsonrpc > **client) > * '*err' if not NULL. */ > int > unixctl_client_transact(struct jsonrpc *client, const char *command, int > argc, > - char *argv[], char **result, char **err) > + char *argv[], struct json **result, struct json > **err) > { > struct jsonrpc_msg *request, *reply; > struct json **json_args, *params; > @@ -506,26 +612,17 @@ unixctl_client_transact(struct jsonrpc *client, const > char *command, int argc, > return error; > } > > - if (reply->error) { > - if (reply->error->type == JSON_STRING) { > - *err = xstrdup(json_string(reply->error)); > - } else { > - VLOG_WARN("%s: unexpected error type in JSON RPC reply: %s", > - jsonrpc_get_name(client), > - json_type_to_string(reply->error->type)); > - error = EINVAL; > - } > - } else if (reply->result) { > - if (reply->result->type == JSON_STRING) { > - *result = xstrdup(json_string(reply->result)); > - } else { > - VLOG_WARN("%s: unexpected result type in JSON rpc reply: %s", > - jsonrpc_get_name(client), > - json_type_to_string(reply->result->type)); > - error = EINVAL; > - } > + if (reply->result && reply->error) { > + VLOG_WARN("unexpected response when communicating with %s: %s\n %s", > + jsonrpc_get_name(client), > + json_to_string(reply->result, JSSF_SORT), > + json_to_string(reply->error, JSSF_SORT)); > + return EINVAL; We'll leak the reply here. Should assign the error code to 'error' and move the clones below under else condition. > } > > + *result = json_nullable_clone(reply->result); > + *err = json_nullable_clone(reply->error); > + > jsonrpc_msg_destroy(reply); > return error; > } > diff --git a/lib/unixctl.h b/lib/unixctl.h > index 4562dbc49..85d2000e0 100644 > --- a/lib/unixctl.h > +++ b/lib/unixctl.h > @@ -17,10 +17,21 @@ > #ifndef UNIXCTL_H > #define UNIXCTL_H 1 > > +#include <stdbool.h> > + > #ifdef __cplusplus > extern "C" { > #endif > > +struct json; > +enum unixctl_output_fmt { > + UNIXCTL_OUTPUT_FMT_TEXT = 1 << 0, > + UNIXCTL_OUTPUT_FMT_JSON = 1 << 1, > +}; > + > +const char *unixctl_output_fmt_to_string(enum unixctl_output_fmt); > +bool unixctl_output_fmt_from_string(const char *, enum unixctl_output_fmt *); > + > /* Server for Unix domain socket control connection. */ > struct unixctl_server; > int unixctl_server_create(const char *path, struct unixctl_server **); > @@ -36,7 +47,7 @@ int unixctl_client_create(const char *path, struct jsonrpc > **client); > int unixctl_client_transact(struct jsonrpc *client, > const char *command, > int argc, char *argv[], > - char **result, char **error); > + struct json **result, struct json **error); > > /* Command registration. */ > struct unixctl_conn; > @@ -45,8 +56,12 @@ typedef void unixctl_cb_func(struct unixctl_conn *, > void unixctl_command_register(const char *name, const char *usage, > int min_args, int max_args, > unixctl_cb_func *cb, void *aux); > +enum unixctl_output_fmt > +unixctl_command_get_output_format(struct unixctl_conn *); Should be enum unixctl_output_fmt unixctl_command_get_output_format( struct unixctl_conn *); i.e. prototype name should not be on a separate line. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
