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.
> > 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.
>
O.K. Thanks.
>
> > 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.
>
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.
>
> > 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.
>
> > > - /* 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
