[ovs-dev] [threads v2 13/13] vlog: Make thread-safe.

Ben Pfaff blp at nicira.com
Wed Jul 17 18:42:56 UTC 2013


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.

> also:
> lib/bfd.c:461:52: warning: restricted __be64 degrades to integer
> lib/bfd.c:461:68: warning: restricted __be32 degrades to integer
> May be not related to this change.

The latter "bfd" warnings are unrelated.  I pushed a fix earlier this
morning.

> 2 additional comments inline. Other changes looks good.
> 
> On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <blp at 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.

> 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?

> > -    /* Skip re-opening if it would be a no-op because the old and new
> > files are
> > -     * the same.  (This avoids writing "closing log file" followed
> > immediately
> > -     * by "opened log file".) */
> > -    if (log_fd >= 0
> > -        && !fstat(log_fd, &old)
> > -        && !stat(log_file_name, &new)
> > -        && old.st_dev == new.st_dev
> > -        && old.st_ino == new.st_ino) {
> > +    if (log_file_name) {
> >
> We are accessing log_file_name outside of the lock, why not just check fn?

Oops, I changed this code a dozen times and got confused.  I've now
changed this to use 'fn'.



More information about the dev mailing list