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();
>>> }
>>>
>>> + 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
