Thx for the review,
I adopted all your suggestions,
I'm extending the bash completion to maybe all ovs-* cmds except ovs-vsctl.
I'm changing the 'struct command' like this:
diff --git a/lib/command-line.h b/lib/command-line.h
index 157cb58..57fdff5 100644
--- a/lib/command-line.h
+++ b/lib/command-line.h
@@ -27,7 +27,7 @@ struct command {
const char *name;
+ const char *usage;
int min_args;
int max_args;
- void (*handler)(int argc, char *argv[]);
+ void (*handler)(int argc, char *argv[], void *aux);
};
The 'usage' will record the format of cmds. (e.g. the 'usage' in 'struct
unixctl_command'). The 'help' subcommand will print the command with
usage and bash completion script will parse it.
The 'handler()' change is for eliminating the 'struct dpctl_command'.
I saw this will affect a ton of cmd source files e.g. ovsdb-tool,
ovs-benchmark
which uses 'struct command'.
So, want to know what you think about doing this.
Thanks
Alex Wang,
On Mon, Sep 15, 2014 at 9:01 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Thu, Sep 11, 2014 at 05:38:21PM -0700, Alex Wang wrote:
> > This commit adds function that extracts (both long and short)
> > options and returns them as string. Right now, only ovs-appctl
> > command provides an option to print this function output. And
> > the output is to be used by bash command-line completion script
> > for completion other options of the same command.
> >
> > Signed-off-by: Alex Wang <al...@nicira.com>
>
> The output format doesn't distinguish between optional and required
> options. Should it?
>
> > + ret = xstrdup(ds_cstr(&ds));
> > + ds_destroy(&ds);
> > +
> > + return ret;
>
> I would replace the above by just "return ds.string;"
>
> > static void
> > +print_appctl_options(const struct option options[])
> > +{
> > + char *opts;
> > +
> > + opts = print_options(options);
> > + printf("%s", opts);
> > + free(opts);
> > +
> > + exit(EXIT_SUCCESS);
> > +}
>
> print_appctl_options() plus print_options() makes for a flexible
> interface, but do you expect that there will be other users that don't
> want to what print_appctl_options() does? I'd be inclined toward just
> making print_options() do what print_appctl_options() does and skip the
> extra wrapper.
>
> None of this matters that much so:
> Acked-by: Ben Pfaff <b...@nicira.com>
> but I don't want it to be applied until the command-line completion code
> is also reviewed by someone.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
