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.
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.
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.
There is another log_fd access around line 985 without lock.
BTW, can we annotate variable access that needs to be protected by a lock?
> - async_append_destroy(log_writer);
> - log_writer = NULL;
> -
> + /* Close old log file, if any, and install new one. */
> + xpthread_mutex_lock(&log_file_mutex);
> + if (log_fd >= 0) {
> + free(log_file_name);
> close(log_fd);
> - log_fd = -1;
> + async_append_destroy(log_writer);
> }
>
> /* Closes and then attempts to re-open the current log file. (This is
> useful
> @@ -327,25 +360,19 @@ vlog_set_log_file(const char *file_name)
> int
> vlog_reopen_log_file(void)
> {
> - struct stat old, new;
> + char *fn;
>
> - /* Skip re-opening if there's nothing to reopen. */
> - if (!log_file_name) {
> - return 0;
> - }
> + xpthread_mutex_lock(&log_file_mutex);
> + fn = log_file_name ? xstrdup(log_file_name) : NULL;
> + xpthread_mutex_unlock(&log_file_mutex);
>
> - /* 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?
+ int error = vlog_set_log_file(fn);
> + free(fn);
> + return error;
> + } else {
> return 0;
> }
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
