Looks good to me. Ethan
On Tue, Jul 26, 2011 at 10:13, Ben Pfaff <[email protected]> wrote: > On Mon, Jul 25, 2011 at 02:51:54PM -0700, Ethan Jackson wrote: >> Nifty! I like this patch. >> >> > - ? ?if (argc != 1) { >> > + ? ?switch (argc) { >> > + ? ?case 0: >> > + ? ? ? ?return xasprintf("unix:%s/db.sock", ovs_rundir()); >> > + >> > + ? ?case 1: >> > + ? ? ? ?return argv[0]; >> > + >> > + ? ?default: >> > ? ? ? ? VLOG_FATAL("database socket is only non-option argument; " >> > ? ? ? ? ? ? ? ? ? ?"use --help for usage"); >> > ? ? } >> >> I'll make the same comment here that I did in the previous patch about >> xstrdup-ing argv[0]. > > I made the same change here, thanks. > >> Also I think the VLOG_FATAL message needs to be updated. > > I think it's still technically correct, but I changed it to "at most one > non-option argument accepted". Is that more helpful? > > I noticed that the --help output needed an update. > > Here's an incremental. I won't wait for re-review, but please do point > out mistakes for me to fix later. > > diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c > index 07dcd28..7d4e4d7 100644 > --- a/vswitchd/ovs-vswitchd.c > +++ b/vswitchd/ovs-vswitchd.c > @@ -53,7 +53,7 @@ VLOG_DEFINE_THIS_MODULE(vswitchd); > > static unixctl_cb_func ovs_vswitchd_exit; > > -static const char *parse_options(int argc, char *argv[]); > +static char *parse_options(int argc, char *argv[]); > static void usage(void) NO_RETURN; > > int > @@ -61,7 +61,7 @@ main(int argc, char *argv[]) > { > struct unixctl_server *unixctl; > struct signal *sighup; > - const char *remote; > + char *remote; > bool exiting; > int retval; > > @@ -83,6 +83,8 @@ main(int argc, char *argv[]) > unixctl_command_register("exit", ovs_vswitchd_exit, &exiting); > > bridge_init(remote); > + free(remote); > + > exiting = false; > while (!exiting) { > if (signal_poll(sighup)) { > @@ -108,7 +110,7 @@ main(int argc, char *argv[]) > return 0; > } > > -static const char * > +static char * > parse_options(int argc, char *argv[]) > { > enum { > @@ -196,10 +198,10 @@ parse_options(int argc, char *argv[]) > return xasprintf("unix:%s/db.sock", ovs_rundir()); > > case 1: > - return argv[0]; > + return xstrdup(argv[0]); > > default: > - VLOG_FATAL("database socket is only non-option argument; " > + VLOG_FATAL("at most one non-option argument accepted; " > "use --help for usage"); > } > } > @@ -208,9 +210,10 @@ static void > usage(void) > { > printf("%s: Open vSwitch daemon\n" > - "usage: %s [OPTIONS] DATABASE\n" > - "where DATABASE is a socket on which ovsdb-server is > listening.\n", > - program_name, program_name); > + "usage: %s [OPTIONS] [DATABASE]\n" > + "where DATABASE is a socket on which ovsdb-server is listening\n" > + " (default: \"unix:%s/db.sock\").\n", > + program_name, program_name, ovs_rundir()); > stream_usage("DATABASE", true, false, true); > daemon_usage(); > vlog_usage(); > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
