Sorry for the post-push comment, took a look over it, seems reasonable enough. One question:<br>+ while True:<br>
+ if not idl.run():<br>
+ continue<br>+ <other stuff><br>+ ...<br>I think idl.run is a fairly fast check itself, will this tight of a loop burn a lot of cycles?<br><br>If so, I'll use my winnings to purchase one xen-optimization nitpick =)<br>
+ for n in session.xenapi.network.get_all():<br>
+ rec = session.xenapi.network.get_record(n)<br>You can use session.xenapi.network.get_all_records(), since you don't use the reference directly anyways.<br><br> -Reid<br><br><div class="gmail_quote">On Wed, Aug 25, 2010 at 5:00 PM, Justin Pettit <span dir="ltr"><<a href="mailto:jpettit@nicira.com">jpettit@nicira.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im">On Aug 25, 2010, at 4:36 PM, Ben Pfaff wrote:<br>
<br>
> Since this modifies the xenserver directory it needs a Signed-off-by:<br>
> line. (Maybe some of the other patches too?)<br>
<br>
</div>Okay, I went back and did that to the three patches that touched XenServer files.<br>
<div class="im"><br>
>> + # Start daemon to monitor external ids<br>
>> + export PYTHONPATH=/usr/share/openvswitch/python<br>
>> + /usr/share/openvswitch/scripts/monitor-external-ids \<br>
>> + --pidfile --detach "$VSWITCHD_OVSDB_SERVER"<br>
><br>
> I would write<br>
> PYTHONPATH=/usr/share/openvswitch/python \<br>
> /usr/share/openvswitch/scripts/monitor-external-ids \<br>
> --pidfile --detach "$VSWITCHD_OVSDB_SERVER"<br>
> instead, to ensure that PYTHONPATH doesn't "leak" into anything else<br>
> that runs after this in the init script. Probably doesn't matter,<br>
> but...<br>
<br>
</div>I had meant to do that before sending it out. Thanks!<br>
<div class="im"><br>
>> + if [ -e /var/run/openvswitch/monitor-external-ids.pid ]; then<br>
>> + kill `cat /var/run/openvswitch/monitor-external-ids.pid`<br>
>> + rm /var/run/openvswitch/monitor-external-ids.pid<br>
>> + fi<br>
><br>
> Doesn't the stop_daemon function work here? Oh, I see, we don't set a<br>
> _PIDFILE variable for the monitor daemon. Maybe we should. (And then<br>
> we'd need --pidfile-name=$whatever_PIDFILE when we start the daemon.)<br>
<br>
</div>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.<br>
<div class="im"><br>
> I kind of zoned out reading a lot of this Python code. I've read and<br>
> written a lot of Python lately and can't quite maintain my attention.<br>
><br>
> I assume it works.<br>
<br>
<br>
</div>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!"<br>
<br>
Thanks for the review. I also made the minor changes you had mentioned and pushed.<br>
<font color="#888888"><br>
--Justin<br>
</font><div><div></div><div class="h5"><br>
<br>
<br>
_______________________________________________<br>
dev mailing list<br>
<a href="mailto:dev@openvswitch.org">dev@openvswitch.org</a><br>
<a href="http://openvswitch.org/mailman/listinfo/dev_openvswitch.org" target="_blank">http://openvswitch.org/mailman/listinfo/dev_openvswitch.org</a><br>
</div></div></blockquote></div><br>