[ovs-dev] [PATCH 2/2] lockfile: Log more helpful message when locking fails due to a conflict.

[Ben Pfaff]

I'll add him to AUTHORS.

In this case, I got the spelling and capitalization directly from
rahim's original email.  I feel that it is polite to spell and
capitalize a person's name as he does himself.  What do you think?

Thanks,

Ben.

On Thu, Aug 30, 2012 at 04:56:47PM -0700, Ethan Jackson wrote:
> Will you please capitalize Rahim's name in the Reported-By? Should he
> be added to authors? Looks like a win, thanks.
> 
> Acked-by: Ethan Jackson <ethan at nicira.com>
> Ethan
> 
> 
> On Thu, Aug 30, 2012 at 3:29 PM, Ben Pfaff <blp at nicira.com> wrote:
> > Reported-by: rahim entezari <rahim.entezari at gmail.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  lib/lockfile.c    |   21 ++++++++++++++++-----
> >  tests/lockfile.at |    7 ++++---
> >  2 files changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/lockfile.c b/lib/lockfile.c
> > index db84aeb..3708aec 100644
> > --- a/lib/lockfile.c
> > +++ b/lib/lockfile.c
> > @@ -55,7 +55,8 @@ struct lockfile {
> >  static struct hmap lock_table = HMAP_INITIALIZER(&lock_table);
> >
> >  static void lockfile_unhash(struct lockfile *);
> > -static int lockfile_try_lock(const char *name, struct lockfile **lockfilep);
> > +static int lockfile_try_lock(const char *name, pid_t *pidp,
> > +                             struct lockfile **lockfilep);
> >
> >  /* Returns the name of the lockfile that would be created for locking a file
> >   * named 'filename_'.  The caller is responsible for freeing the returned name,
> > @@ -98,21 +99,27 @@ lockfile_lock(const char *file, struct lockfile **lockfilep)
> >       * stylized ways such that any number of readers may access a file while it
> >       * is being written. */
> >      char *lock_name;
> > +    pid_t pid;
> >      int error;
> >
> >      COVERAGE_INC(lockfile_lock);
> >
> >      lock_name = lockfile_name(file);
> >
> > -    error = lockfile_try_lock(lock_name, lockfilep);
> > +    error = lockfile_try_lock(lock_name, &pid, lockfilep);
> >
> >      if (error) {
> >          COVERAGE_INC(lockfile_error);
> >          if (error == EACCES) {
> >              error = EAGAIN;
> >          }
> > -        VLOG_WARN("%s: failed to lock file: %s",
> > -                  lock_name, strerror(error));
> > +        if (pid) {
> > +            VLOG_WARN("%s: cannot lock file because it is already locked by "
> > +                      "pid %ld", lock_name, (long int) pid);
> > +        } else {
> > +            VLOG_WARN("%s: failed to lock file: %s",
> > +                      lock_name, strerror(error));
> > +        }
> >      }
> >
> >      free(lock_name);
> > @@ -201,7 +208,7 @@ lockfile_register(const char *name, dev_t device, ino_t inode, int fd)
> >  }
> >
> >  static int
> > -lockfile_try_lock(const char *name, struct lockfile **lockfilep)
> > +lockfile_try_lock(const char *name, pid_t *pidp, struct lockfile **lockfilep)
> >  {
> >      struct flock l;
> >      struct stat s;
> > @@ -209,6 +216,7 @@ lockfile_try_lock(const char *name, struct lockfile **lockfilep)
> >      int fd;
> >
> >      *lockfilep = NULL;
> > +    *pidp = 0;
> >
> >      /* Check whether we've already got a lock on that file. */
> >      if (!stat(name, &s)) {
> > @@ -250,6 +258,9 @@ lockfile_try_lock(const char *name, struct lockfile **lockfilep)
> >      if (!error) {
> >          *lockfilep = lockfile_register(name, s.st_dev, s.st_ino, fd);
> >      } else {
> > +        if (!fcntl(fd, F_GETLK, &l) && l.l_type != F_UNLCK) {
> > +            *pidp = l.l_pid;
> > +        }
> >          close(fd);
> >      }
> >      return error;
> > diff --git a/tests/lockfile.at b/tests/lockfile.at
> > index 50999e4..2644d3f 100644
> > --- a/tests/lockfile.at
> > +++ b/tests/lockfile.at
> > @@ -5,7 +5,8 @@ m4_define([CHECK_LOCKFILE],
> >     AT_KEYWORDS([lockfile])
> >     AT_CHECK([test-lockfile $1], [0], [$1: success (m4_if(
> >       [$2], [1], [$2 child], [$2 children]))
> > -], [$3])
> > +], [stderr])
> > +   AT_CHECK([sed 's/pid [[0-9]]*/pid <pid>/' stderr], [0], [$3])
> >     AT_CLEANUP])
> >
> >  CHECK_LOCKFILE([lock_and_unlock], [0])
> > @@ -23,13 +24,13 @@ lockfile|WARN|.file.~lock~: failed to lock file: Resource deadlock avoided
> >
> >  CHECK_LOCKFILE([lock_blocks_other_process], [1],
> >    [lockfile|WARN|.file.~lock~: child does not inherit lock
> > -lockfile|WARN|.file.~lock~: failed to lock file: Resource temporarily unavailable
> > +lockfile|WARN|.file.~lock~: cannot lock file because it is already locked by pid <pid>
> >  ])
> >
> >  CHECK_LOCKFILE([lock_twice_blocks_other_process], [1],
> >    [lockfile|WARN|.file.~lock~: failed to lock file: Resource deadlock avoided
> >  lockfile|WARN|.file.~lock~: child does not inherit lock
> > -lockfile|WARN|.file.~lock~: failed to lock file: Resource temporarily unavailable
> > +lockfile|WARN|.file.~lock~: cannot lock file because it is already locked by pid <pid>
> >  ])
> >
> >  CHECK_LOCKFILE([lock_and_unlock_allows_other_process], [1])
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev