[ovs-dev] [PATCH] Make ovs-appctl easier to use and synchronize its interface with ovs-vsctl.

[Justin Pettit]

I'm very sorry it's taken me so long to get you feedback for this.  I  
think this is a huge usability improvement to ovs-appctl.  Currently,  
it seems targeted at the master branch.  Since this changes the  
calling convention of ovs-appctl, do you think it's worth porting it  
to the "citrix" branch"?

Some feedback follows:

On Sep 17, 2009, at 11:26 AM, Ben Pfaff wrote:

> +Tells \fBovs\-appctl\fR which daemon to contact.
> +.IP
> +If \fItarget\fR begins with \fB/\fR it must name a Unix domain socket
> +on which an Open vSwitch daemon is listening for control channel
> +connections.  By default, each daemon listens on a Unix domain socket
> +named \fB at RUNDIR@/\fIprogram\fB.\fIpid\fR.ctl\fR, where \fIprogram\fR

I think "\fR.ctl" should be "\fB.ctl"

> +is the program's name and \fIpid\fR is its process ID.  For example,
> +if \fBovs-vswitchd\fR has PID 123, it would listen on
> +\fB at RUNDIR@/ovs-vswitchd.123.ctl\fR.
> +.IP
> +Otherwise, \fBovs\-appctl\fR looks for a pidfile, that is, a file
> +whose contents are the process ID of a running process as a decimal
> +number, named \fB at RUNDIR@/\fItarget\fB.pid\fR.  (The \fB\-\-pidfile 
> \fR
> +option makes an Open vSwitch daemon create a pidfile.)
> +\fBovs\-appctl\fR reads the pidfile, then looks for a Unix socket
> +named \fB at RUNDIR@/\fItarget\fR.\fIpid\fR.ctl\fR, where \fIpid\fR is

The same here.

> +replaced by the process ID read from \fItarget\fR, and uses that file

It's not really read from "\fItarget\fR", right"?  What about  
something like "\fItarget\fR's pidfile"?

> +.SH BUGS
> +
> +The protocol used to speak to Open vSwitch daemons does not contain a
> +quoting mechanism, so command arguments should not generally contain
> +white space.

Is that true?  There are quite a few that seem to use white space  
(e.g., bond/*), and one gets around that by quoting in the arguments.

> +
> .SH "SEE ALSO"

The "SEE ALSO" section has sort of a random set of man pages.  I'd  
think we should at least provide pointers to the binaries that are  
controllable by ovs-appctl and have description of the supported  
commands in their man pages.

> +    printf("%s, for querying and controlling Open vSwitch daemon\n"
> +           "usage: %s [TARGET] COMMAND [ARG...]\n"
> +           "Targets:\n"
> +           "  -t, --target=TARGET  pidfile or socket to contact\n"
> +           "Common commands:"
> +           "  vlog/list          List current logging levels\n"
> +           "  vlog/set MODULE[:FACILITY[:LEVEL]]\n"
> +           "        Set MODULE and FACILITY log level to LEVEL\n"
> +           "        MODULE may be any valid module name or 'ANY'\n"
> +           "        FACILITY may be 'syslog', 'console', 'file', or  
> 'ANY' (default)\n"
> +           "        LEVEL may be 'emer', 'err', 'warn', 'info', or  
> 'dbg' (default)\n"
> +           "  vlog/reopen        Make the program reopen its log  
> file\n"

Should we require that "help" be implemented by any daemon  
controllable by ovs-appctl?  I'm afraid that a quick reading of the  
output may not give a user how best to proceed.  Something like this  
may be helpful:

	"  help     List commands supported by the target"

> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index b70f6c8..cd383c4 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
...
> +If \fItarget\fR begins with \fB/\fR it must name a Unix domain socket
> +on which \fBovs\-vswitchd\fR is listening for control channel
> +connections.  By default, \fBovs\-vswitchd\fR listens on a Unix  
> domain
> +socket named \fB at RUNDIR@/ovs\-vswitchd.\fIpid\fR.ctl\fR, where

I think "\fR.ctl" should be "\fB.ctl"

> -If \fItarget\fR does not begin with \fB/\fR, then \fB at RUNDIR@/\fR is
> -implicitly prefixed to it.
> +The default target is \fBovs\-vswitchd\fR.
> .IP
> If neither \fB\-t\fR nor \fB\-\-target\fR is specified, the default  
> target is
> \fB at RUNDIR@/ovs\-vswitchd.pid\fR.

I believe this sentence is now redundant due to the previous one.   
However, I think it's actually clearer, so I'd recommend removing "The  
default target is \fBovs\-vswitchd\fR." line.

> diff --git a/utilities/ovs-vsctl.in b/utilities/ovs-vsctl.in
> index 675f9dd..7270d01 100755
> --- a/utilities/ovs-vsctl.in
> +++ b/utilities/ovs-vsctl.in
> @@ -30,7 +30,7 @@ if argv0.find('/') >= 0:
> DEFAULT_VSWITCHD_CONF = "@sysconfdir@/ovs-vswitchd.conf"
> VSWITCHD_CONF = DEFAULT_VSWITCHD_CONF
>
> -DEFAULT_VSWITCHD_TARGET = "@RUNDIR@/ovs-vswitchd.pid"
> +DEFAULT_VSWITCHD_TARGET = "ovs-vswitchd"
> VSWITCHD_TARGET = DEFAULT_VSWITCHD_TARGET

"VSWITCHD_TARGET" isn't treated as a constant, so I think it would be  
clearer if you only started it with a capital letter to indicate that  
it's a global variable.  This goes for a few other global variables  
being used, too, such as VSWITCHD_CONF, RELOAD_VSWITCHD, and SYSLOG.

Thanks for improving on my original patch!

--Justin