[ovs-dev,v1,ovn] ovn-nb/sbctl.c: Use env variables for passing options.
diff mbox series

Message ID 20191009235640.36470-1-amginwal@gmail.com
State New
Headers show
Series
  • [ovs-dev,v1,ovn] ovn-nb/sbctl.c: Use env variables for passing options.
Related show

Commit Message

aginwala aginwala Oct. 9, 2019, 11:56 p.m. UTC
From: Aliasgar Ginwala <aginwala@ebay.com>

Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
ovn-nbctl and ovn-sbctl respectively where user can set any single
supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
Above env var OVN_NBCTL_OPTIONS have no effect if user runs
command as ovn-nbctl --no-leader-only <command>

Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
---
 utilities/ovn-nbctl.8.xml |  7 +++++++
 utilities/ovn-nbctl.c     | 36 ++++++++++++++++++++++++++++++++++--
 utilities/ovn-sbctl.8.in  |  6 ++++++
 utilities/ovn-sbctl.c     | 36 ++++++++++++++++++++++++++++++++++--
 4 files changed, 81 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Oct. 14, 2019, 10:33 p.m. UTC | #1
On Wed, Oct 09, 2019 at 04:56:40PM -0700, amginwal@gmail.com wrote:
> From: Aliasgar Ginwala <aginwala@ebay.com>
> 
> Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> ovn-nbctl and ovn-sbctl respectively where user can set any single
> supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> command as ovn-nbctl --no-leader-only <command>
> 
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>

I think that this could be factored out into lib/command-line.c rather
than done as a copy-and-paste into two files.

I don't think that the ops_passed variable needs to be static.
Ben Pfaff Oct. 14, 2019, 10:39 p.m. UTC | #2
On Mon, Oct 14, 2019 at 03:33:25PM -0700, Ben Pfaff wrote:
> On Wed, Oct 09, 2019 at 04:56:40PM -0700, amginwal@gmail.com wrote:
> > From: Aliasgar Ginwala <aginwala@ebay.com>
> > 
> > Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> > ovn-nbctl and ovn-sbctl respectively where user can set any single
> > supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> > Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> > command as ovn-nbctl --no-leader-only <command>
> > 
> > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> 
> I think that this could be factored out into lib/command-line.c rather
> than done as a copy-and-paste into two files.
> 
> I don't think that the ops_passed variable needs to be static.

Also, I'd consider giving an example in the documentation of how to use
this new variable.  The purpose is unclear enough that I might not be
able to quickly guess how or why to use it without an example.
aginwala Oct. 15, 2019, 2:45 a.m. UTC | #3
On Mon, Oct 14, 2019 at 4:38 PM Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Oct 14, 2019 at 03:33:25PM -0700, Ben Pfaff wrote:
> > On Wed, Oct 09, 2019 at 04:56:40PM -0700, amginwal@gmail.com wrote:
> > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > >
> > > Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> > > ovn-nbctl and ovn-sbctl respectively where user can set any single
> > > supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> > > Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> > > command as ovn-nbctl --no-leader-only <command>
> > >
> > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> >
> > I think that this could be factored out into lib/command-line.c rather
> > than done as a copy-and-paste into two files.
> >
> > I don't think that the ops_passed variable needs to be static.
>
> Also, I'd consider giving an example in the documentation of how to use
> this new variable.  The purpose is unclear enough that I might not be
> able to quickly guess how or why to use it without an example.
>
Thanks for the review Ben:
I can move that logic to command-line.c to new function in ovs repo
as ovs_cmdl_env_parse_all and use it in nbctl and sbctl.   Will update
example in doc accordingly in v2.

> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Oct. 15, 2019, 4:20 p.m. UTC | #4
On Mon, Oct 14, 2019 at 07:45:11PM -0700, aginwala wrote:
> On Mon, Oct 14, 2019 at 4:38 PM Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Mon, Oct 14, 2019 at 03:33:25PM -0700, Ben Pfaff wrote:
> > > On Wed, Oct 09, 2019 at 04:56:40PM -0700, amginwal@gmail.com wrote:
> > > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > > >
> > > > Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> > > > ovn-nbctl and ovn-sbctl respectively where user can set any single
> > > > supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> > > > Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> > > > command as ovn-nbctl --no-leader-only <command>
> > > >
> > > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > >
> > > I think that this could be factored out into lib/command-line.c rather
> > > than done as a copy-and-paste into two files.
> > >
> > > I don't think that the ops_passed variable needs to be static.
> >
> > Also, I'd consider giving an example in the documentation of how to use
> > this new variable.  The purpose is unclear enough that I might not be
> > able to quickly guess how or why to use it without an example.
> >
> Thanks for the review Ben:
> I can move that logic to command-line.c to new function in ovs repo
> as ovs_cmdl_env_parse_all and use it in nbctl and sbctl.   Will update
> example in doc accordingly in v2.

Thanks!
aginwala Oct. 16, 2019, 1:54 a.m. UTC | #5
On Tue, Oct 15, 2019 at 10:38 AM Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Oct 14, 2019 at 07:45:11PM -0700, aginwala wrote:
> > On Mon, Oct 14, 2019 at 4:38 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > > On Mon, Oct 14, 2019 at 03:33:25PM -0700, Ben Pfaff wrote:
> > > > On Wed, Oct 09, 2019 at 04:56:40PM -0700, amginwal@gmail.com wrote:
> > > > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > > > >
> > > > > Add new env variables OVN_NBCTL_OPTIONS and OVN_SBCTL_OPTIONS for
> > > > > ovn-nbctl and ovn-sbctl respectively where user can set any single
> > > > > supported option. e.g export OVN_NBCTL_OPTIONS=--no-leader-only.
> > > > > Above env var OVN_NBCTL_OPTIONS have no effect if user runs
> > > > > command as ovn-nbctl --no-leader-only <command>
> > > > >
> > > > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > > >
> > > > I think that this could be factored out into lib/command-line.c
> rather
> > > > than done as a copy-and-paste into two files.
> > > >
> > > > I don't think that the ops_passed variable needs to be static.
> > >
> > > Also, I'd consider giving an example in the documentation of how to use
> > > this new variable.  The purpose is unclear enough that I might not be
> > > able to quickly guess how or why to use it without an example.
> > >
> > Thanks for the review Ben:
> > I can move that logic to command-line.c to new function in ovs repo
> > as ovs_cmdl_env_parse_all and use it in nbctl and sbctl.   Will update
> > example in doc accordingly in v2.
>
> Thanks!
>
Thanks Ben. Sent https://patchwork.ozlabs.org/patch/1177492/ and
https://patchwork.ozlabs.org/patch/1177492/ to handle the same. PTAL.

Patch
diff mbox series

diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index fd75c0e44..6a7962973 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -1178,6 +1178,13 @@ 
           wait at all.  Use the <code>sync</code> command to override this
           behavior.
         </p>
+
+        <p>
+          If the <env>OVN_NBCTL_OPTIONS</env> environment variable is set,
+          its value is used as the default to set above options. If user
+          passes options via cli, <env>OVN_NBCTL_OPTIONS</env> environment
+          variable will have no effect.
+        </p>
       </dd>
 
     <dt><code>--db</code> <var>database</var></dt>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index a89a9cb4d..8d2bc0968 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -124,18 +124,49 @@  static char * OVS_WARN_UNUSED_RESULT main_loop(const char *args,
 static void server_loop(struct ovsdb_idl *idl, int argc, char *argv[]);
 
 int
-main(int argc, char *argv[])
+main(int argc, char *argv_[])
 {
     struct ovsdb_idl *idl;
     struct shash local_options;
 
-    set_program_name(argv[0]);
+    set_program_name(argv_[0]);
     fatal_ignore_sigpipe();
     vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
     vlog_set_levels_from_string_assert("reconnect:warn");
 
     nbctl_cmd_init();
 
+    /* Check if options are set via env var. */
+    static int ops_passed = false;
+    int i, j = 0;
+    char *ovn_nbctl_options = getenv("OVN_NBCTL_OPTIONS");
+    char **argv = xcalloc(argc + 1, sizeof( *argv_) + 1);
+    if (ovn_nbctl_options) {
+        for (i = 0; i < argc; i++) {
+            if (strcmp(argv_[i], ovn_nbctl_options) == 0) {
+                ops_passed = true;
+                break;
+            }
+        }
+        /* if option not passed via cli, read env var set by user.*/
+        if (!ops_passed) {
+            for (i = 0, j = 0; i < argc; i++, j++) {
+                if (j == 1) {
+                    argv[j] = ovn_nbctl_options;
+                    j++;
+                }
+                argv[j] = xstrdup(argv_[i]);
+            }
+            argc = j;
+        }
+    }
+    if (ops_passed || !ovn_nbctl_options) {
+        /* Copy args for parsing as is from argv_ */
+        for (i = 0; i < argc; i++) {
+            argv[i] = xstrdup(argv_[i]);
+        }
+    }
+
     /* ovn-nbctl has three operation modes:
      *
      *    - Direct: Executes commands by contacting ovsdb-server directly.
@@ -240,6 +271,7 @@  main(int argc, char *argv[])
     idl = the_idl = NULL;
 
     free(args);
+    free(argv);
     exit(EXIT_SUCCESS);
 }
 
diff --git a/utilities/ovn-sbctl.8.in b/utilities/ovn-sbctl.8.in
index 2aaa457e8..b9cfde897 100644
--- a/utilities/ovn-sbctl.8.in
+++ b/utilities/ovn-sbctl.8.in
@@ -93,6 +93,12 @@  to approximately \fIsecs\fR seconds.  If the timeout expires,
 would normally happen only if the database cannot be contacted, or if
 the system is overloaded.)
 .
+.IP "\fBOVN_SBCTL_OPTIONS\fR"
+If \fBOVN_SBCTL_OPTIONS\fR environment variable is set,
+its value is used as the default to set above options. If user
+passes options via cli, \fBOVN_SBCTL_OPTIONS\fR environment
+variable will have no effect.
+.
 .so lib/vlog.man
 .so lib/common.man
 .
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index 9a9b6f0ec..51bfe7101 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -93,7 +93,7 @@  static bool do_sbctl(const char *args, struct ctl_command *, size_t n,
                      struct ovsdb_idl *);
 
 int
-main(int argc, char *argv[])
+main(int argc, char *argv_[])
 {
     struct ovsdb_idl *idl;
     struct ctl_command *commands;
@@ -101,13 +101,44 @@  main(int argc, char *argv[])
     unsigned int seqno;
     size_t n_commands;
 
-    set_program_name(argv[0]);
+    set_program_name(argv_[0]);
     fatal_ignore_sigpipe();
     vlog_set_levels(NULL, VLF_CONSOLE, VLL_WARN);
     vlog_set_levels_from_string_assert("reconnect:warn");
 
     sbctl_cmd_init();
 
+    /* Check if options are set via env var. */
+    static int ops_passed = false;
+    int i, j = 0;
+    char *ovn_sbctl_options = getenv("OVN_SBCTL_OPTIONS");
+    char **argv = xcalloc(argc + 1, sizeof( *argv_) + 1);
+    if (ovn_sbctl_options) {
+        for (i = 0; i < argc; i++) {
+            if (strcmp(argv_[i], ovn_sbctl_options) == 0) {
+                ops_passed = true;
+                break;
+            }
+        }
+        /* if option not passed via cli, read env var set by user.*/
+        if (!ops_passed) {
+            for (i = 0, j = 0; i < argc; i++, j++) {
+                if (j == 1) {
+                    argv[j] = ovn_sbctl_options;
+                    j++;
+                }
+                argv[j] = xstrdup(argv_[i]);
+            }
+            argc = j;
+        }
+    }
+    if (ops_passed || !ovn_sbctl_options) {
+        /* Copy args for parsing as is from argv_ */
+        for (i = 0; i < argc; i++) {
+            argv[i] = xstrdup(argv_[i]);
+        }
+    }
+
     /* Parse command line. */
     char *args = process_escape_args(argv);
     shash_init(&local_options);
@@ -147,6 +178,7 @@  main(int argc, char *argv[])
             seqno = ovsdb_idl_get_seqno(idl);
             if (do_sbctl(args, commands, n_commands, idl)) {
                 free(args);
+                free(argv);
                 exit(EXIT_SUCCESS);
             }
         }