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