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)
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
