----- Original Message -----
> From: "Ben Pfaff" <b...@ovn.org>
> To: "Lance Richardson" <lrich...@redhat.com>
> Cc: "Joe Stringer" <j...@ovn.org>, dev@openvswitch.org, "Paul Boca"
> <pb...@cloudbasesolutions.com>
> Sent: Friday, June 3, 2016 2:31:30 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:49:54PM -0400, Lance Richardson wrote:
> > ----- Original Message -----
> > > From: "Ben Pfaff" <b...@ovn.org>
> > > To: "Lance Richardson" <lrich...@redhat.com>
> > > Cc: "Joe Stringer" <j...@ovn.org>, dev@openvswitch.org, "Paul Boca"
> > > <pb...@cloudbasesolutions.com>
> > > 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.
>
Another thought I had while looking at this: since
OVS_APP_EXIT_AND_WAIT_BY_TARGET
is already in place, and since it differs from OVS_APP_EXIT_AND_WAIT only in
(1) using a unixctl socket for the target and (2) using a specific rule for
the name of the pidfile, we could rework OVS_APP_EXIT_AND_WAIT_BY_TARGET to
take a unixctl socket and a pidfile for parameters and use the reworked
version for both purposes.
OVS_APP_EXIT_AND_WAIT_BY_TARGET is currently only used in 3 spots.
I *think* this would be a fairly small set of changes, although from the above
diff it seems some cleanup is needed in addition to fixing this specific
problem.
Lance
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
