Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On 19 Mar 2024, at 11:09, Jakob Meng wrote:
> On 15.03.24 11:15, Eelco Chaudron wrote:
>> [...]
>> 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
>
> Thanks for your review ☺️ Some questions below..
>
> Best,
> Jakob
>
>>> 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.
>
> The error handling of this set-options call is intentionally kept very
> similar to the error handling of the later command call. Should I drop the
> "free(cmd_error);" in both sections because we exit anyway? Should I move it
> up in both cases?
>
>> Should we not exit on a transaction error also? I think error handling might
>> need some more cleanup.
>
> We exit on transaction error with ovs_fatal?! Just like in the next command
> call..
>
> What kind of cleanup do you have in mind?
I come from the pre-MMU era, where exiting without freeing memory meant losing
it forever. With MMUs in place, I suppose it's acceptable to leave this code
unchanged. My attempts to tidy it up only made it less readable.
>>> +}
>>> +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();
>>> }
>>>
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On 15.03.24 11:15, Eelco Chaudron wrote:
> [...]
> 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
Thanks for your review ☺️ Some questions below..
Best,
Jakob
>> 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.
The error handling of this set-options call is intentionally kept very similar
to the error handling of the later command call. Should I drop the
"free(cmd_error);" in both sections because we exit anyway? Should I move it up
in both cases?
> Should we not exit on a transaction error also? I think error handling might
> need some more cleanup.
We exit on transaction error with ovs_fatal?! Just like in the next command
call..
What kind of cleanup do you have in mind?
>> +}
>> +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/reopenMake 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',
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:
> From: Jakob Meng
>
> 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
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
> "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";
> +
> +cas
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On Thu, Jan 18, 2024 at 04:39:56PM +, Simon Horman wrote: > On Thu, Jan 18, 2024 at 04:26:52PM +0100, jm...@redhat.com wrote: > > From: Jakob Meng > > > > For monitoring systems such as Prometheus it would be beneficial if > > OVS would expose statistics in a machine-readable format. > > ... > > Recheck-request: github-robot Rerun was successful :) https://github.com/ovsrobot/ovs/actions/runs/7572473760 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
On Thu, Jan 18, 2024 at 04:26:52PM +0100, jm...@redhat.com wrote: > From: Jakob Meng > > For monitoring systems such as Prometheus it would be beneficial if > OVS would expose statistics in a machine-readable format. ... Recheck-request: github-robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
Bleep bloop. Greetings Jakob Meng, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line lacks whitespace around operator WARNING: Line lacks whitespace around operator #592 FILE: utilities/ovs-appctl.c:144: -f, --format=FMT Output format. One of: 'json', or 'text'\n\ Lines checked: 699, Warnings: 2, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v7 1/6] Add global option for JSON output to ovs-appctl.
From: Jakob Meng
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
---
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
"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_
