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/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) {
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
