On Thu, Apr 24, 2014 at 11:03:16PM -0700, Andy Zhou wrote:
> Acked-by: Andy Zhou <az...@nicira.com>
>
> With a question inline.
>
>
> On Thu, Apr 24, 2014 at 4:59 PM, Ben Pfaff <b...@nicira.com> wrote:
> > If the 'lacp' parameter is nonnull, then we know that the file scope mutex
> > has been initialized, since that's done as a side effect of creating a
> > lacp object, but otherwise there's no guarantee.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > lib/lacp.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/lacp.c b/lib/lacp.c
> > index 4aee64f..49ae5e5 100644
> > --- a/lib/lacp.c
> > +++ b/lib/lacp.c
> > @@ -345,18 +345,18 @@ out:
> > enum lacp_status
> > lacp_status(const struct lacp *lacp) OVS_EXCLUDED(mutex)
> > {
> > - enum lacp_status ret;
> > + if (lacp) {
> > + enum lacp_status ret;
> >
> > - ovs_mutex_lock(&mutex);
> > - if (!lacp) {
> > - ret = LACP_DISABLED;
> > - } else if (lacp->negotiated) {
> > - ret = LACP_NEGOTIATED;
> > + ovs_mutex_lock(&mutex);
> > + ret = lacp->negotiated ? LACP_NEGOTIATED : LACP_CONFIGURED;
> > + ovs_mutex_unlock(&mutex);
> > + return ret;
> > } else {
> > - ret = LACP_CONFIGURED;
> > + /* Don't take 'mutex'. It might not even be initialized, since we
> > + * don't know that any lacp object has been created. */
> > + return LACP_DISABLED;
> This would have been hard to understand without the comment above.
> Thanks for adding them.
> I am curious why not just initialize the mutex in lacp_init()?
Hmm. lacp_init() isn't necessarily run at the right time, since it's
only to set up unixctl commands.
But just initializing more carefully does seem like a better solution.
I'll respin.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
