| Message ID | 20191009235640.36470-1-amginwal@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v1,ovn] ovn-nb/sbctl.c: Use env variables for passing options. | expand |
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.
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.
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 >
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!
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.
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); } }