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

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

Commit Message

aginwala aginwala Oct. 16, 2019, 1:52 a.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
supported ovn-nb/sbctl options using environment variable.
e.g. OVN_SBCTL_OPTIONS="--db=unix:sb1.ovsdb --no-leader-only"

Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
---

v1 -> v2
-------
  * Addressed Ben's comment - Write ovs_cmdl_env_parse_all for parsing
  env options in lib/command-line.c and update docs about new env var
  usage.

 utilities/ovn-nbctl.8.xml | 25 +++++++++++++++++++++++++
 utilities/ovn-nbctl.c     | 19 +++++++++++++++++--
 utilities/ovn-sbctl.8.in  |  8 ++++++++
 utilities/ovn-sbctl.c     | 19 +++++++++++++++++--
 4 files changed, 67 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Oct. 25, 2019, 6:24 p.m. UTC | #1
On Tue, Oct 15, 2019 at 06:52:52PM -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
> supported ovn-nb/sbctl options using environment variable.
> e.g. OVN_SBCTL_OPTIONS="--db=unix:sb1.ovsdb --no-leader-only"
> 
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> +    /* Check if options are set via env var. */
> +    char *ctl_options = getenv("OVN_NBCTL_OPTIONS");
> +    char **argv;
> +    int *argcp;
> +    argcp = xmalloc(sizeof(int));
> +    *argcp = argc;
> +    argv = ovs_cmdl_env_parse_all(argcp, argv_,
> +                                  ctl_options);
> +    if (!argv) {
> +        /* This situation should never occur, but... */
> +        ctl_fatal("Unable to read argv");
> +    }
> +    argc = *argcp;
> +    free(argcp);

This seems to me to be more complicated than necessary.  Is the
following sufficient?

argv = ovs_cmdl_env_parse_all(&argc, argv, getenv("OVN_NBCTL_OPTIONS"));

I don't know why the check for null argv is needed.
Nitin katiyar via dev Oct. 25, 2019, 8:12 p.m. UTC | #2
Yeah makes sense since we are doing null check in command-line. Thanks for the review and suggestions. Sent v3 https://patchwork.ozlabs.org/patch/1184436/ PTAL.

Patch
diff mbox series

diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index fd75c0e44..b207dac46 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -1178,6 +1178,31 @@ 
           wait at all.  Use the <code>sync</code> command to override this
           behavior.
         </p>
+
+        <p>
+          User can set one or more <env>OVN_NBCTL_OPTIONS</env> options in
+          environment variable. Under the Bourne shell this might be
+          done like this:
+        </p>
+
+        <pre fixed="yes">
+          OVN_NBCTL_OPTIONS="--db=unix:nb1.ovsdb --no-leader-only"
+        </pre>
+
+        <p>
+          When <env>OVN_NBCTL_OPTIONS</env> is set, <code>ovn-nbctl</code>
+          automatically and transparently uses the environment variable to
+          execute its commands. However user can still over-ride environment
+          options by passing different in cli.
+        </p>
+
+        <p>
+          When the environment variable is no longer needed, unset it, e.g.:
+        </p>
+
+        <pre fixed="yes">
+          unset OVN_NBCTL_OPTIONS
+        </pre>
       </dd>
 
     <dt><code>--db</code> <var>database</var></dt>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index a89a9cb4d..684f0f40b 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -124,18 +124,33 @@  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. */
+    char *ctl_options = getenv("OVN_NBCTL_OPTIONS");
+    char **argv;
+    int *argcp;
+    argcp = xmalloc(sizeof(int));
+    *argcp = argc;
+    argv = ovs_cmdl_env_parse_all(argcp, argv_,
+                                  ctl_options);
+    if (!argv) {
+        /* This situation should never occur, but... */
+        ctl_fatal("Unable to read argv");
+    }
+    argc = *argcp;
+    free(argcp);
+
     /* ovn-nbctl has three operation modes:
      *
      *    - Direct: Executes commands by contacting ovsdb-server directly.
diff --git a/utilities/ovn-sbctl.8.in b/utilities/ovn-sbctl.8.in
index 2aaa457e8..b3c21d625 100644
--- a/utilities/ovn-sbctl.8.in
+++ b/utilities/ovn-sbctl.8.in
@@ -93,6 +93,14 @@  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"
+.User can set one or more options using \fBOVN_SBCTL_OPTIONS\fR environment
+.variable. Under the Bourne shell this might be done like this:
+.export \fBOVN_SBCTL_OPTIONS\fR"="--db=unix:sb1.ovsdb --no-leader-only".
+.However user can still over-ride environment options by passing different
+.options in cli. When the environment variable is no longer needed, unset it,
+.e.g.: unset \fBOVN_SBCTL_OPTIONS\fR"
+.
 .so lib/vlog.man
 .so lib/common.man
 .
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index 9a9b6f0ec..7c991a5fc 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,28 @@  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. */
+    char *ctl_options = getenv("OVN_SBCTL_OPTIONS");
+    char **argv;
+    int *argcp;
+    argcp = xmalloc(sizeof(int));
+    *argcp = argc;
+    argv = ovs_cmdl_env_parse_all(argcp, argv_,
+                                  ctl_options);
+    if (!argv) {
+        /* This situation should never occur, but... */
+        ctl_fatal("Unable to read argv");
+    }
+    argc = *argcp;
+    free(argcp);
+
     /* Parse command line. */
     char *args = process_escape_args(argv);
     shash_init(&local_options);