[ovs-dev] [PATCH V3 6/10] ofproto-dpif-monitor: Run ofproto-dpif-monitor in a thread.

[Alex Wang]

Thanks for the review!


> +/* latch that controls the exit of monitor thread. */
>  > +static struct latch monitor_exit_latch;
>
> I'm not sure this comment is adding much that isn't clear from the
> variable's type and name.  But if you think it's necessary, perhaps it
> would be better as simply "Causes exit of the monitor thread."
>
> > +/* "struct seq" to wakeup the monitor thread. */
> > +static struct seq *monitor_seq;
>
> Similarly, if you think its necessary "Wakes up the monitor thread.",
> it's clear that it's a seq from the type.
>


Yeah, I'll remove those comments.



>
> Do we really need separate monitor_check_run(), monitor_start(), and
> monitor_terminate() functions?  I don't think splitting them out is
> making the code any clearer.  Why not just combine them into
> ofproto_dpif_monitor_port_update()?
>
>

I'll combine them up.



> I don't really like that we're heap allocating the tid just to keep
> track of whether or not the thread is running.  How about a 'running'
> bool?
>


I can use a global value.  but the tid is necessary in that we need to call
"join()" and wait for the termination of monitor thread.



> There's some comments "zalloc tid" "frees the tid" which aren't
> telling us anything that isn't obvious from the code.
>


I'll delete them.



> The bfd unit test changes should probably be in their own patch.
>


 I agree, it is pretty big change when I look at it now.  so, I'll resend
these patches, without following the current version number. (since now we
have one more patch)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20131014/b1dc479b/attachment-0003.html>