[ovs-dev] [PATCH v2] command-line.c: Support parsing ctl options via env variable

Ginwala, Aliasgar aginwala at ebay.com
Fri Oct 25 18:51:54 UTC 2019


Sure, let me change and test it. Will send out v3 addressing the same.
________________________________
From: Ben Pfaff <blp at ovn.org>
Sent: Friday, October 25, 2019 11:47 AM
To: Ilya Maximets <i.maximets at ovn.org>
Cc: ovs-dev at openvswitch.org <ovs-dev at openvswitch.org>; Ginwala, Aliasgar <aginwala at ebay.com>
Subject: Re: Re: [ovs-dev] [PATCH v2] command-line.c: Support parsing ctl options via env variable

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?


More information about the dev mailing list