On Wed, Jul 17, 2013 at 01:11:38PM -0700, Andy Zhou wrote:
> On Wed, Jul 17, 2013 at 11:42 AM, Ben Pfaff <b...@nicira.com> wrote:
>
> > On Tue, Jul 16, 2013 at 06:11:56PM -0700, Andy Zhou wrote:
> > > Review of 11-13 of this patch series.
> > >
> > > Got a bunch of spare error: warning: Using plain integer as NULL
> > pointer,.
> > > Most seem to be caused VLOG_RATE_LIMIT_INIT.
> >
> > Does your tree contain Ethan's patch "atomic: Suppress sparse warning."?
> > It is on current master. I believe that this should avoid the sparse
> > warnings.
> >
> I ran this based on the github review tree.
Ah. I keep updating that but I don't know what it contained when you
fetched it. It could have been before that patch was merged in.
> > > On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <b...@nicira.com> wrote:
> > >
> > > > - /* Close old log file. */
> > > > + /* Log closing old log file (we can't log while holding
> > > > log_file_mutex). */
> > > > if (log_fd >= 0) {
> > > > VLOG_INFO("closing log file");
> > > > + }
> > > >
> > > > log_fd may be changed by other thread, so ideally it should be checked
> > > under log_file_mutex. It may be safe to check for (log_fd >= 0) without
> > > worry about race conditions, but on the other hand, I don't see a reason
> > > not to do it under the mutex.
> >
> > The comment says why: we can't log while holding log_file_mutex (because
> > logging can also take log_file_mutex). We could do various dodges, but
> > it doesn't really seem worth it just to avoid a race on whether we log a
> > message about closing a log file.
> >
> I got it for VLOG_INFO, but it seems we could have cached the value of
> log_fd while holding the lock. It is not a big deal, but seems nicer in
> case we support APIs to close a log file without opening a new one
> immediately.
We could cache it, but I don't think that it would help.
> >
> > > There is another log_fd access around line 985 without lock.
> >
> > vlog.c only has 985 lines in my tree here, so can you be more specific?
> >
> > > BTW, can we annotate variable access that needs to be protected by a
> > lock?
> >
> > What kind of annotations do you have in mind?
> >
> Annotate a variable's access should be done within a lock. Not sure if it
> is possible with static analysis tools.
I don't think that sparse can do anything useful here.
Ethan says that Clang can do some static analysis on locks. I think
he's working on annotations. I don't know whether the Clang analysis
just covers balanced acquire and release of locks, or if it can actually
tell what objects need to be locked.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
