On Fri, Jun 03, 2016 at 12:49:54PM -0400, Lance Richardson wrote:
> ----- Original Message -----
> > From: "Ben Pfaff" <[email protected]>
> > To: "Lance Richardson" <[email protected]>
> > Cc: "Joe Stringer" <[email protected]>, [email protected], "Paul Boca"
> > <[email protected]>
> > Sent: Friday, June 3, 2016 12:38:56 PM
> > Subject: Re: [ovs-dev] [PATCH V2 4/4] tests: Fix fail of
> > OVS_APP_EXIT_AND_WAIT on Windows
> >
> > On Fri, Jun 03, 2016 at 12:00:11PM -0400, Lance Richardson wrote:
> > > OK, it appears that I was probably a little too focused on the ~250
> > > locations where OVS_APP_EXIT_AND_WAIT() is invoked with a daemon
> > > name (where the TMPPID stuff works fine) and not enough to the ~8
> > > locations where it's invoked with the path of a unixctl socket.
> > >
> > > Ben's suggestion to have the Windows-specfic kill() function emulate
> > > the kill command's handling of "kill -0" should make this macro
> > > behave as it does on other systems, but I'm wondering if it might
> > > also be good to create a separate macro for the unixctl case. Ideally
> > > there would be a way to derive the pid from the unixctl socket path,
> > > but I believe we can't assume the existence of things like fuser or
> > > lsof even on Linux or BSD systems. Or maybe this variant could
> > > take both the unixctl socket path and pid file path as parameters.
> >
> > The "kill -0" change is a tangent. It solves a particular problem, but
> > not the root of this issue.
> >
> > It's a mistake to pass a unixctl socket name to OVS_APP_EXIT_AND_WAIT.
>
> Right.
>
> > A new macro is one possible solution. However, looking at a few of the
> > cases where OVS_APP_EXIT_AND_WAIT is being passed a unixctl socket, I'm
> > not sure why we couldn't just pass a pidfile instead. Some of the users
> > don't have a pidfile, but why not just add one?
> >
>
> I'm fine with that, the main difference between creating a new macro and
> adding a parameter to the existing one is the number of invocation sites
> that would need to be modified... 8 or so if a new macro is created and
> over 250 if a parameter is added to the existing macro. It's more work
> in the short term to add a parameter, but it does have a cleaner end result.
>
> I'll go ahead and start working a patch set up to add a pidfile parameter
> to OVS_APP_EXIT_AND_WAIT.
I don't think we need a new parameter to the existing function. I think
that we just need those 8 cases to create pidfiles, e.g.
diff --git a/tests/ovsdb-monitor.at b/tests/ovsdb-monitor.at
index 37383fa..48275f0 100644
--- a/tests/ovsdb-monitor.at
+++ b/tests/ovsdb-monitor.at
@@ -24,23 +24,25 @@ m4_define([OVSDB_CHECK_MONITOR],
m4_foreach([txn], [$3],
[AT_CHECK([ovsdb-tool transact db 'txn'], [0], [ignore], [ignore])])
AT_CAPTURE_FILE([ovsdb-server-log])
- AT_CHECK([ovsdb-server --detach --no-chdir --pidfile="`pwd`"/server-pid
--remote=punix:socket --unixctl="`pwd`"/unixctl
--log-file="`pwd`"/ovsdb-server-log db >/dev/null 2>&1],
+ AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --remote=punix:socket
--log-file="`pwd`"/ovsdb-server-log db >/dev/null 2>&1],
[0], [], [])
+ on_exit "kill `cat ovsdb-server.pid`"
if test "$IS_WIN32" = "yes"; then
- AT_CHECK([ovsdb-client -vjsonrpc --pidfile="`pwd`"/client-pid -d json
monitor --format=csv unix:socket $4 $5 $8 > output &],
- [0], [ignore], [ignore], [kill `cat server-pid`])
+ AT_CHECK([ovsdb-client -vjsonrpc --pidfile -d json monitor --format=csv
unix:socket $4 $5 $8 > output &],
+ [0], [ignore], [ignore])
sleep 1
else
- AT_CHECK([ovsdb-client -vjsonrpc --detach --no-chdir
--pidfile="`pwd`"/client-pid -d json monitor --format=csv unix:socket $4 $5 $8
> output],
- [0], [ignore], [ignore], [kill `cat server-pid`])
+ AT_CHECK([ovsdb-client -vjsonrpc --detach --no-chdir --pidfile -d json
monitor --format=csv unix:socket $4 $5 $8 > output],
+ [0], [ignore], [ignore])
fi
+ on_exit "kill `cat ovsdb-client.pid`"
m4_foreach([txn], [$6],
[AT_CHECK([ovsdb-client transact unix:socket 'txn'], [0],
- [ignore], [ignore], [kill `cat server-pid client-pid`])])
+ [ignore], [ignore])])
AT_CHECK([ovsdb-client transact unix:socket '[["$4"]]'], [0],
- [ignore], [ignore], [kill `cat server-pid client-pid`])
- OVS_APP_EXIT_AND_WAIT(["`pwd`"/unixctl])
- OVS_WAIT_UNTIL([test ! -e client-pid])
+ [ignore], [ignore])
+ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+ OVS_WAIT_UNTIL([test ! -e ovsdb-client.pid])
AT_CHECK([${PERL} $srcdir/ovsdb-monitor-sort.pl < output | ${PERL}
$srcdir/uuidfilt.pl], [0], [$7], [ignore])
AT_CLEANUP])
Some cases are a little harder and might benefit from something new, but
other cases, like the above, just seem to underuse or misuse existing
infrastructure.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
