[ovs-dev] [PATCH 5/5] xenserver: Add monitor-external-ids daemon

[Justin Pettit]

On Aug 25, 2010, at 4:36 PM, Ben Pfaff wrote:

> Since this modifies the xenserver directory it needs a Signed-off-by:
> line.  (Maybe some of the other patches too?)

Okay, I went back and did that to the three patches that touched XenServer files.

>> +    # Start daemon to monitor external ids
>> +    export PYTHONPATH=/usr/share/openvswitch/python
>> +    /usr/share/openvswitch/scripts/monitor-external-ids \
>> +                 --pidfile --detach "$VSWITCHD_OVSDB_SERVER"
> 
> I would write
>        PYTHONPATH=/usr/share/openvswitch/python \
>            /usr/share/openvswitch/scripts/monitor-external-ids \
>            --pidfile --detach "$VSWITCHD_OVSDB_SERVER"
> instead, to ensure that PYTHONPATH doesn't "leak" into anything else
> that runs after this in the init script.  Probably doesn't matter,
>        but...

I had meant to do that before sending it out.  Thanks!

>> +    if [ -e /var/run/openvswitch/monitor-external-ids.pid ]; then
>> +        kill `cat /var/run/openvswitch/monitor-external-ids.pid`
>> +        rm /var/run/openvswitch/monitor-external-ids.pid
>> +    fi
> 
> Doesn't the stop_daemon function work here?  Oh, I see, we don't set a
> _PIDFILE variable for the monitor daemon.  Maybe we should.  (And then
> we'd need --pidfile-name=$whatever_PIDFILE when we start the daemon.)

Yeah, that's why I didn't do it.  I plan to clean this part up, but we were in a rush to get this out.

> I kind of zoned out reading a lot of this Python code.  I've read and
> written a lot of Python lately and can't quite maintain my attention.
> 
> I assume it works.


That was the least enthusiastic review of code I've ever seen.  At least when I pretend to review vconn and ovsdb code, I have the dignity to lie about it.  "Looks awesome!"  "Fascinating!"  "I love me some self-generating code!"

Thanks for the review.  I also made the minor changes you had mentioned and pushed.

--Justin