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

[Reid Price]

Sorry for the post-push comment, took a look over it, seems reasonable
enough.  One question:
+    while True:
+        if not idl.run():
+            continue
+       <other stuff>
+   ...
I think idl.run is a fairly fast check itself, will this tight of a loop
burn a lot of cycles?

If so, I'll use my winnings to purchase one xen-optimization nitpick =)
+    for n in session.xenapi.network.get_all():
+        rec = session.xenapi.network.get_record(n)
You can use session.xenapi.network.get_all_records(), since you don't use
the reference directly anyways.

  -Reid

On Wed, Aug 25, 2010 at 5:00 PM, Justin Pettit <jpettit at nicira.com> wrote:

> 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
>
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100827/07e4ed96/attachment-0003.html>