| Message ID | 20191025054314.31258-1-amginwal@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] command-line.c: Support parsing ctl options via env variable | expand |
On Thu, Oct 24, 2019 at 10:43:14PM -0700, amginwal@gmail.com wrote: > From: Aliasgar Ginwala <aginwala@ebay.com> > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com> To me, this looks more complicated than necessary, and thus harder to reason about regarding correctness. What about the following? It does not make me think as hard about correctness. However, I have not tested it. -8<--------------------------cut here-------------------------->8-- From: Aliasgar Ginwala <aginwala@ebay.com> Date: Thu, 24 Oct 2019 22:43:14 -0700 Subject: [PATCH] command-line.c: Support parsing ctl options via env variable Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com> Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/command-line.c | 34 ++++++++++++++++++++++++++++++++++ lib/command-line.h | 3 +++ 2 files changed, 37 insertions(+) diff --git a/lib/command-line.c b/lib/command-line.c index 9e000bd28d1a..fa02b49152e5 100644 --- a/lib/command-line.c +++ b/lib/command-line.c @@ -19,6 +19,7 @@ #include <getopt.h> #include <limits.h> #include <stdlib.h> +#include "svec.h" #include "openvswitch/dynamic-string.h" #include "ovs-thread.h" #include "util.h" @@ -77,6 +78,39 @@ find_option_by_value(const struct option *options, int value) return NULL; } +/* Parses options set using environment variable. The caller specifies the + * supported options in environment variable. On success, adds the parsed + * env variables in 'argv', the number of options in 'argc', and returns argv. + */ +char ** +ovs_cmdl_env_parse_all(int *argcp, char *argv[], const char *env_options) +{ + struct svec args = SVEC_EMPTY_INITIALIZER; + + /* argv[0] stays in place. */ + ovs_assert(*argcp > 0); + svec_add(&args, argv[0]); + + /* Anything from the environment variable goes next. */ + if (env_options) { + char *env_copy = xstrdup(env_options); + char *save_ptr = NULL; + for (char *option = strtok_r(env_copy, " ", &save_ptr); + option; option = strtok_r(NULL, " ", &save_ptr)) { + svec_add(&args, option); + } + free(env_copy); + } + + /* Remaining command-line options go at the end. */ + for (int i = 1; i < *argcp; i++) { + svec_add(&args, argv[i]); + } + + *argcp = args.n; + return args.names; +} + /* Parses the command-line options in 'argc' and 'argv'. The caller specifies * the supported options in 'options'. On success, stores the parsed options * in '*pop', the number of options in '*n_pop', and returns NULL. On failure, diff --git a/lib/command-line.h b/lib/command-line.h index 9d62dc2501c5..dc7ec36eae6c 100644 --- a/lib/command-line.h +++ b/lib/command-line.h @@ -54,6 +54,9 @@ char *ovs_cmdl_parse_all(int argc, char *argv[], const struct option *, struct ovs_cmdl_parsed_option **, size_t *) OVS_WARN_UNUSED_RESULT; +char **ovs_cmdl_env_parse_all(int *argcp, char *argv[], + const char *env_options); + void ovs_cmdl_print_options(const struct option *options); void ovs_cmdl_print_commands(const struct ovs_cmdl_command *commands);
Thanks Ben: It works fine. Just a minor typo. It's much neat and trimmed now. I tried with multiple options e.g. export OVN_NBCTL_OPTIONS="--db=unix:nb1.ovsdb --no-leader-only --leader-only --no-leader-only" and it gets called as expected: Called as ovn-nbctl --db=unix:nb1.ovsdb --no-leader-only --leader-only --no-leader-only set-connection pssl:6641
> Thanks Ben: It works fine. Just a minor typo. It's much neat and trimmed now. > > I tried with multiple options e.g. > export OVN_NBCTL_OPTIONS="--db=unix:nb1.ovsdb --no-leader-only --leader-only --no-leader-only" > > and it gets called as expected: > Called as ovn-nbctl --db=unix:nb1.ovsdb --no-leader-only --leader-only --no-leader-only set-connection pssl:6641 > > ________________________________ > From: Ben Pfaff <blp at ovn.org> > Sent: Friday, October 25, 2019 11:12 AM > To: amginwal at gmail.com <amginwal at gmail.com> > Cc: dev at openvswitch.org <dev at openvswitch.org>; Ginwala, Aliasgar <aginwala at ebay.com> > Subject: Re: [ovs-dev] [PATCH v2] command-line.c: Support parsing ctl options via env variable > > On Thu, Oct 24, 2019 at 10:43:14PM -0700, amginwal at gmail.com wrote: >> From: Aliasgar Ginwala <aginwala at ebay.com> >> >> Signed-off-by: Aliasgar Ginwala <aginwala at ebay.com> > > To me, this looks more complicated than necessary, and thus harder to > reason about regarding correctness. What about the following? It does > not make me think as hard about correctness. However, I have not tested > it. > > -8<--------------------------cut here-------------------------->8-- > > From: Aliasgar Ginwala <aginwala at ebay.com> > Date: Thu, 24 Oct 2019 22:43:14 -0700 > Subject: [PATCH] command-line.c: Support parsing ctl options via env variable > > Signed-off-by: Aliasgar Ginwala <aginwala at ebay.com> > Signed-off-by: Ben Pfaff <blp at ovn.org> > --- > lib/command-line.c | 34 ++++++++++++++++++++++++++++++++++ > lib/command-line.h | 3 +++ > 2 files changed, 37 insertions(+) > > diff --git a/lib/command-line.c b/lib/command-line.c > index 9e000bd28d1a..fa02b49152e5 100644 > --- a/lib/command-line.c > +++ b/lib/command-line.c > @@ -19,6 +19,7 @@ > #include <getopt.h> > #include <limits.h> > #include <stdlib.h> > +#include "svec.h" > #include "openvswitch/dynamic-string.h" > #include "ovs-thread.h" > #include "util.h" > @@ -77,6 +78,39 @@ find_option_by_value(const struct option *options, int value) > return NULL; > } > > +/* Parses options set using environment variable. The caller specifies the > + * supported options in environment variable. On success, adds the parsed > + * env variables in 'argv', the number of options in 'argc', and returns argv. > + */ > +char ** > +ovs_cmdl_env_parse_all(int *argcp, char *argv[], const char *env_options) > +{ > + struct svec args = SVEC_EMPTY_INITIALIZER; > + > + /* argv[0] stays in place. */ > + ovs_assert(*argcp > 0); > + svec_add(&args, argv[0]); > Should be argv_ > + > + /* Anything from the environment variable goes next. */ > + if (env_options) { > + char *env_copy = xstrdup(env_options); > + char *save_ptr = NULL; > + for (char *option = strtok_r(env_copy, " ", &save_ptr); > + option; option = strtok_r(NULL, " ", &save_ptr)) { > + svec_add(&args, option); > + } > + free(env_copy); Just curious, can we use svec_parse_words() instead of above loop? Best regards, Ilya Maximets.
On Fri, Oct 25, 2019 at 08:42:46PM +0200, Ilya Maximets wrote: > > Thanks Ben: It works fine. Just a minor typo. It's much neat and trimmed now. > > > > I tried with multiple options e.g. > > export OVN_NBCTL_OPTIONS="--db=unix:nb1.ovsdb --no-leader-only --leader-only --no-leader-only" > > > > and it gets called as expected: > > Called as ovn-nbctl --db=unix:nb1.ovsdb --no-leader-only --leader-only --no-leader-only set-connection pssl:6641 > > > > ________________________________ > > From: Ben Pfaff <blp at ovn.org> > > Sent: Friday, October 25, 2019 11:12 AM > > To: amginwal at gmail.com <amginwal at gmail.com> > > Cc: dev at openvswitch.org <dev at openvswitch.org>; Ginwala, Aliasgar <aginwala at ebay.com> > > Subject: Re: [ovs-dev] [PATCH v2] command-line.c: Support parsing ctl options via env variable > > > > On Thu, Oct 24, 2019 at 10:43:14PM -0700, amginwal at gmail.com wrote: > > > From: Aliasgar Ginwala <aginwala at ebay.com> > > > > > > Signed-off-by: Aliasgar Ginwala <aginwala at ebay.com> > > > > To me, this looks more complicated than necessary, and thus harder to > > reason about regarding correctness. What about the following? It does > > not make me think as hard about correctness. However, I have not tested > > it. > > > > -8<--------------------------cut here-------------------------->8-- > > > > From: Aliasgar Ginwala <aginwala at ebay.com> > > Date: Thu, 24 Oct 2019 22:43:14 -0700 > > Subject: [PATCH] command-line.c: Support parsing ctl options via env variable > > > > Signed-off-by: Aliasgar Ginwala <aginwala at ebay.com> > > Signed-off-by: Ben Pfaff <blp at ovn.org> > > --- > > lib/command-line.c | 34 ++++++++++++++++++++++++++++++++++ > > lib/command-line.h | 3 +++ > > 2 files changed, 37 insertions(+) > > > > diff --git a/lib/command-line.c b/lib/command-line.c > > index 9e000bd28d1a..fa02b49152e5 100644 > > --- a/lib/command-line.c > > +++ b/lib/command-line.c > > @@ -19,6 +19,7 @@ > > #include <getopt.h> > > #include <limits.h> > > #include <stdlib.h> > > +#include "svec.h" > > #include "openvswitch/dynamic-string.h" > > #include "ovs-thread.h" > > #include "util.h" > > @@ -77,6 +78,39 @@ find_option_by_value(const struct option *options, int value) > > return NULL; > > } > > > > +/* Parses options set using environment variable. The caller specifies the > > + * supported options in environment variable. On success, adds the parsed > > + * env variables in 'argv', the number of options in 'argc', and returns argv. > > + */ > > +char ** > > +ovs_cmdl_env_parse_all(int *argcp, char *argv[], const char *env_options) > > +{ > > + struct svec args = SVEC_EMPTY_INITIALIZER; > > + > > + /* argv[0] stays in place. */ > > + ovs_assert(*argcp > 0); > > + svec_add(&args, argv[0]); > > Should be argv_ > > + > > + /* Anything from the environment variable goes next. */ > > + if (env_options) { > > + char *env_copy = xstrdup(env_options); > > + char *save_ptr = NULL; > > + for (char *option = strtok_r(env_copy, " ", &save_ptr); > > + option; option = strtok_r(NULL, " ", &save_ptr)) { > > + svec_add(&args, option); > > + } > > + free(env_copy); > > Just curious, can we use svec_parse_words() instead of above loop? That would actually be much better, since it handles quoting. (I wrote svec_parse_words() but I forgot about it!) Ali, can you make that change?
Sure, let me change and test it. Will send out v3 addressing the same.
diff --git a/lib/command-line.c b/lib/command-line.c index 9e000bd28..d13cca294 100644 --- a/lib/command-line.c +++ b/lib/command-line.c @@ -19,6 +19,7 @@ #include <getopt.h> #include <limits.h> #include <stdlib.h> +#include "svec.h" #include "openvswitch/dynamic-string.h" #include "ovs-thread.h" #include "util.h" @@ -77,6 +78,60 @@ find_option_by_value(const struct option *options, int value) return NULL; } +/* Parses options set using environment variable. The caller specifies the + * supported options in environment variable. On success, adds the parsed + * env variables in 'argv', the number of options in 'argc', and returns argv. + * */ +char ** +ovs_cmdl_env_parse_all(int *argcp, char *argv_[], + char *env_options) +{ + char *str1, *token, *saveptr1; + char **argv = NULL; + int i, j, total_args, argc; + int env_argc = 0; + + argc = *argcp; + if (!env_options) { + /* Copy args for parsing as is from argv_ */ + argv = xcalloc(argc + 1, sizeof( *argv_) + 1 ); + for (i = 0; i < argc; i++) { + argv[i] = xstrdup(argv_[i]); + } + return argv; + } + + /* Count number of options passed via environment variable */ + struct svec env_vars; + svec_init(&env_vars); + for (j = 1, str1 = env_options; ; j++, str1 = NULL) { + token = strtok_r(str1, " ", &saveptr1); + if (token == NULL) { + break; + } + svec_add(&env_vars, token); + env_argc++; + } + total_args = argc + env_argc + 1; + argv = xcalloc(total_args, sizeof( *argv_) + env_argc + 1); + + /* Construct argv with command line + environment options. */ + for (i = 0, j = 0; i < argc; i++, j++) { + if (j == 1) { + const char *env; + size_t k; + SVEC_FOR_EACH (k, env, &env_vars) { + argv[j] = xstrdup(env); + j++; + } + } + argv[j] = xstrdup(argv_[i]); + } + svec_destroy(&env_vars); + *argcp = j; + return argv; +} + /* Parses the command-line options in 'argc' and 'argv'. The caller specifies * the supported options in 'options'. On success, stores the parsed options * in '*pop', the number of options in '*n_pop', and returns NULL. On failure, diff --git a/lib/command-line.h b/lib/command-line.h index 9d62dc250..4b8f76da7 100644 --- a/lib/command-line.h +++ b/lib/command-line.h @@ -54,6 +54,9 @@ char *ovs_cmdl_parse_all(int argc, char *argv[], const struct option *, struct ovs_cmdl_parsed_option **, size_t *) OVS_WARN_UNUSED_RESULT; +char **ovs_cmdl_env_parse_all(int *argcp, char *argv_[], + char *env_options); + void ovs_cmdl_print_options(const struct option *options); void ovs_cmdl_print_commands(const struct ovs_cmdl_command *commands);