On Fri, Jun 03, 2016 at 02:51: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 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.
Makes sense.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
