On Wed, Feb 22, 2023 at 11:24:04AM +0100, Ales Musil wrote:
> On Wed, Feb 22, 2023 at 10:59 AM Simon Horman <simon.hor...@corigine.com>
> wrote:
...
Hi Ales,
> Hi Simon,
> thank you for your review.
you are welcome.
> > > ---
> > > utilities/ovn-dbctl.c | 76 +++++++++++++++++++++++++++----------------
> > > 1 file changed, 48 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
> > > index f0ee0ba05..f96cab64e 100644
> > > --- a/utilities/ovn-dbctl.c
> > > +++ b/utilities/ovn-dbctl.c
> > > @@ -109,6 +109,15 @@ static void server_loop(const struct
> > ovn_dbctl_options *dbctl_options,
> > > struct ovsdb_idl *idl, int argc, char *argv[]);
> > > static void ovn_dbctl_exit(int status);
> > >
> > > +static void
> > > +destroy_argv(int argc, char **argv)
> > > +{
> > > + for (int i = 0; i < argc; i++) {
> > > + free(argv[i]);
> > > + }
> > > + free(argv);
> > > +}
> > > +
> >
> > This destroy_argv seems to be the dual of ovs_cmdl_env_parse_all(),
> > does it belong in lib/command-line.c ?
> >
>
> It might fit more into command-line.c however getting something into ovs
> and then
> reflect that in ovn is way longer process.
Fair enough.
I agree it makes sense to start with an OVN fix.
> > Also, do all other users of ovs_cmdl_env_parse_all() - in both OVS and OVN
> > -
> > unwind correctly on error?
> >
>
> I can see only single usage and that's here. So before this patch the
> answer would be no.
Great, thanks for checking.
> > > int
> > > ovn_dbctl_main(int argc, char *argv[],
> > > const struct ovn_dbctl_options *dbctl_options)
> > > @@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[],
> > > char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(),
> > > &parsed_options,
> > &n_parsed_options);
> > > if (error_s) {
> > > + destroy_argv(argc, argv_);
> > > ctl_fatal("%s", error_s);
> > > }
> > >
> > > @@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[],
> > > bool daemon_mode = false;
> > > if (get_detach()) {
> > > if (argc != optind) {
> > > + destroy_argv(argc, argv_);
> > > ctl_fatal("non-option arguments not supported with --detach
> > "
> > > "(use --help for help)");
> > > }
> > > @@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[],
> > > if (error) {
> > > ovsdb_idl_destroy(idl);
> > > idl = the_idl = NULL;
> > > + destroy_argv(argc, argv_);
> > >
> > > - for (int i = 0; i < argc; i++) {
> > > - free(argv_[i]);
> > > - }
> > > - free(argv_);
> > > ctl_fatal("%s", error);
> > > }
> > >
> > > @@ -239,21 +247,15 @@ cleanup:
> > > }
> > > free(commands);
> > > if (error) {
> > > - for (int i = 0; i < argc; i++) {
> > > - free(argv_[i]);
> > > - }
> > > - free(argv_);
> > > + destroy_argv(argc, argv_);
> > > ctl_fatal("%s", error);
> > > }
> > > }
> > >
> > > ovsdb_idl_destroy(idl);
> > > idl = the_idl = NULL;
> > > + destroy_argv(argc, argv_);
> > >
> > > - for (int i = 0; i < argc; i++) {
> > > - free(argv_[i]);
> > > - }
> > > - free(argv_);
> > > exit(EXIT_SUCCESS);
> > > }
> > >
> > > @@ -1240,27 +1242,31 @@ dbctl_client(const struct ovn_dbctl_options
> > *dbctl_options,
> > >
> > > ctl_timeout_setup(timeout);
> > >
> > > + char *err = NULL;
> >
> > perhaps error_str is a nicer name for this?
> >
> > > + char *cmd_error = NULL;
> > > + char *cmd_result = NULL;
> > > + int exit_status = EXIT_SUCCESS;
> > > struct jsonrpc *client;
> >
> > I'm not sure if reverse xmas tree - longest line to shortest - is
> > a thing for local variable declarations in ovn.
> >
>
> I don't think it's strictly held, but it shouldn't hurt.
Yeah, that's what I was thinking too.
> > > +
> > > int error = unixctl_client_create(socket_name, &client);
> > > if (error) {
> > > - ctl_fatal("%s: could not connect to %s daemon (%s); "
> > > - "unset %s to avoid using daemon",
> > > - socket_name, program_name, ovs_strerror(error),
> > > - dbctl_options->daemon_env_var_name);
> > > + err = xasprintf("%s: could not connect to %s daemon (%s); "
> > > + "unset %s to avoid using daemon",
> > > + socket_name, program_name, ovs_strerror(error),
> > > + dbctl_options->daemon_env_var_name);
> > > + goto cleanup;
> > > }
> > >
> > > - char *cmd_result;
> > > - char *cmd_error;
> > > +
> > > error = unixctl_client_transact(client, "run",
> > > args.n, args.names,
> > > &cmd_result, &cmd_error);
> > > if (error) {
> > > - ctl_fatal("%s: transaction error (%s)",
> > > - socket_name, ovs_strerror(error));
> > > + err = xasprintf("%s: transaction error (%s)",
> > > + socket_name, ovs_strerror(error));
> > > + goto cleanup;
> > > }
> > > - svec_destroy(&args);
> > >
> > > - int exit_status;
> > > if (cmd_error) {
> > > exit_status = EXIT_FAILURE;
> > > fprintf(stderr, "%s: %s", program_name, cmd_error);
> > > @@ -1268,12 +1274,26 @@ dbctl_client(const struct ovn_dbctl_options
> > *dbctl_options,
> > > exit_status = EXIT_SUCCESS;
> > > fputs(cmd_result, stdout);
> > > }
> > > - free(cmd_result);
> > > - free(cmd_error);
> > > - jsonrpc_close(client);
> > > - for (int i = 0; i < argc; i++) {
> > > - free(argv[i]);
> > > +
> > > +cleanup:
> > > + if (err) {
> > > + exit_status = EXIT_FAILURE;
> > > + VLOG_ERR("%s", err);
> > > + ovs_error(0, "%s", err);
> > > + free(err);
> > > }
> > > - free(argv);
> > > +
> > > + if (cmd_result) {
> > > + free(cmd_result);
> > > + }
> > > +
> > > + if (cmd_error) {
> > > + free(cmd_error);
> > > + }
> >
> > I don't think the cmd_result and cmd_error conditions are required
> > as free should do nothing when passed NULL.
> >
>
> Isn't free behavior for NULL platform dependent? I was under the impression
> that it is.
> But I might be mistaken.
Maybe: not so secret fact is that I'm a Linux person.
In any case, perhaps goto labels can be arranged to avoid
the conditionals.
> > > +
> > > + jsonrpc_close(client);
> > > + svec_destroy(&args);
> > > + destroy_argv(argc, argv);
> > > +
> > > exit(exit_status);
> > > }
> >
> > Overall, I think it would be nice to re-arrange things a bit so that:
> >
> > 1. Non-error paths are not covered by conditionals.
> > 2. The code covered by goto labels(s) does not include conditionals.
> > 3. exit_status is always set explicitly, rather than
> > relying on overriding a default value.
> >
> > Something like this (*compile tested only!*).
> > And if so, perhaps a cleanup patch is warranted before the
> > patch to fix the problem at hand.
> >
> > char *cmd_result = NULL;
> > char *cmd_error = NULL;
> > struct jsonrpc *client;
> > int exit_status;
> > char *error_str;
> >
> > int error = unixctl_client_create(socket_name, &client);
> > if (error) {
> > error_str = xasprintf("%s: could not connect to %s daemon (%s); "
> > "unset %s to avoid using daemon",
> > socket_name, program_name,
> > ovs_strerror(error),
> > dbctl_options->daemon_env_var_name);
> > goto log_error;
> > }
> >
> >
> > error = unixctl_client_transact(client, "run",
> > args.n, args.names,
> > &cmd_result, &cmd_error);
> > if (error) {
> > error_str = xasprintf("%s: transaction error (%s)",
> > socket_name, ovs_strerror(error));
> > goto log_error;
> > }
> >
> > if (cmd_error) {
> > fprintf(stderr, "%s: %s", program_name, cmd_error);
> > goto error;
> > }
> >
> > fputs(cmd_result, stdout);
> > exit_status = EXIT_SUCCESS;
> > goto cleanup;
> >
> > log_error:
> > VLOG_ERR("%s", error_str);
> > ovs_error(0, "%s", error_str);
> > free(error_str);
> >
> > error:
> > exit_status = EXIT_FAILURE;
> >
> > cleanup:
> > free(cmd_result);
> > free(cmd_error);
> > jsonrpc_close(client);
> > svec_destroy(&args);
> > destroy_argv(argc, argv);
> >
> > exit(exit_status);
> >
> >
> >
> >
> That looks way better, the initial idea was to do that without too big of a
> change which did not
> work very well.
Understood. And for a fix perhaps a minimal approach is better.
But, OTOH, perhaps the overall delta is similar. I'm not sure.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
