On Aug 14, 2013 7:21 PM, "Mattias Andrée" wrote:

That is ridiculus that NFS does not implement flock, but there is a solution documented in flock(2):

flock() does not lock files over NFS. Use fcntl(2) instead: that does work over NFS, given a sufficiently recent version of Linux and a server which supports locking.

Keyword being Linux. We support BSD and OSX as well. fcntl semantics aren't portable. Again, you're not the first to go down this road.

On Wed, 14 Aug 2013 18:14:20 -0400 Dave Reisner wrote:

...

On Thu, Aug 15, 2013 at 12:03:33AM +0200, Mattias Andrée wrote:

...

This allows for further improvement of pacman where shared locks could also be used, in cases there pacman only reads the database and does not edit it.

File locks also makes it easy to let pacman wait until the block instance has finished, by just not using the LOCK_NB flag. ---

And the question that comes up every time this patch arises: what about NFS?

No, we won't change this.

...

lib/libalpm/be_sync.c | 5 +---- lib/libalpm/handle.c | 26 ++++++++++++++------------ lib/libalpm/handle.h | 2 +- lib/libalpm/trans.c | 7 +------ scripts/pacman-db-upgrade.sh.in | 9 +++++---- scripts/pacman-optimize.sh.in | 9 +++++---- 6 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 0b99684..e7c8c4b 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -284,10 +284,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) cleanup:

- if(_alpm_handle_unlock(handle)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("could not remove lock file %s\n"), - handle->lockfile); - } + _alpm_handle_unlock(handle); free(syncpath); umask(oldmask); return ret; diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..5d1cfb7 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -27,6 +27,7 @@ #include #include #include +#include #include

/* libalpm */ @@ -44,6 +45,7 @@ alpm_handle_t *_alpm_handle_new(void)

CALLOC(handle, 1, sizeof(alpm_handle_t), return NULL); handle->deltaratio = 0.0; + handle->lckstream = -1;

return handle; } @@ -95,7 +97,7 @@ int _alpm_handle_lock(alpm_handle_t *handle) char *dir, *ptr;

ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream == NULL, return 0); + ASSERT(handle->lckstream < 0, return 0);

/* create the dir of the lockfile first */ dir = strdup(handle->lockfile); @@ -110,14 +112,14 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir);

do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + fd = open(handle->lockfile, O_WRONLY | O_CREAT); } while(fd == -1 && errno == EINTR); if(fd >= 0) { - FILE *f = fdopen(fd, "w"); - fprintf(f, "%ld\n", (long)getpid()); - fflush(f); fsync(fd); - handle->lckstream = f; + if(flock(fd, LOCK_EX | LOCK_NB) < 0) { + return -1; + } + handle->lckstream = fd; return 0; } return -1; @@ -127,14 +129,14 @@ int _alpm_handle_lock(alpm_handle_t *handle) int _alpm_handle_unlock(alpm_handle_t *handle) { ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream != NULL, return 0); - - fclose(handle->lckstream); - handle->lckstream = NULL; + ASSERT(handle->lckstream >= 0, return 0);

- if(unlink(handle->lockfile) && errno != ENOENT) { - return -1; + flock(handle->lckstream, LOCK_UN); + if(flock(handle->lckstream, LOCK_EX | LOCK_NB) == 0) { + unlink(handle->lockfile); + flock(handle->lckstream, LOCK_UN); } + handle->lckstream = NULL; return 0; }

diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5e84d58..7ca90ff 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,7 +55,7 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ - FILE *lckstream; /* lock file stream pointer if one exists */ + int lckstream; /* lock file file descriptor if one exists */ alpm_trans_t *trans;

#ifdef HAVE_LIBCURL diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 8d4e0e7..122f35b 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -215,12 +215,7 @@ int SYMEXPORT alpm_trans_release(alpm_handle_t *handle) /* unlock db */ if(!nolock_flag) { - if(_alpm_handle_unlock(handle)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("could not remove lock file %s\n"), - handle->lockfile); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: could not remove lock file %s\n", handle->lockfile); - } + _alpm_handle_unlock(handle); }

return 0; diff --git a/scripts/pacman-db-upgrade.sh.in b/scripts/pacman-db-upgrade.sh.in index a1630c5..20cf035 100644 --- a/scripts/pacman-db-upgrade.sh.in +++ b/scripts/pacman-db-upgrade.sh.in @@ -102,12 +102,12 @@ dbroot="${dbroot%/}" # form the path to our lockfile location lockfile="${dbroot}/db.lck"

-# make sure pacman isn't running -if [[ -f $lockfile ]]; then +# make sure pacman isn't running and stop other instances for starting +exec 100<>"$lockfile" +if ! flock -x --nonblock 100; then + flock --close 100 die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")" fi -# do not let pacman run while we do this -touch "$lockfile"

# pacman-3.4 to 3.5 upgrade - merge depends into desc if [[ $(find "$dbroot"/local -name depends) ]]; then @@ -123,5 +123,6 @@ fi

# remove the lock file rm -f "$lockfile" +flock --close 100

# vim: set ts=2 sw=2 noet: diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in index 47fbb49..8c6e556 100644 --- a/scripts/pacman-optimize.sh.in +++ b/scripts/pacman-optimize.sh.in @@ -114,12 +114,12 @@ dbroot="${dbroot%/}" lockfile="${dbroot}/db.lck" localdb="${dbroot}/local"

-# make sure pacman isn't running -if [[ -f $lockfile ]]; then +# make sure pacman isn't running and stop other instances for starting +exec 100<>"$lockfile" +if ! flock -x --nonblock 100; then + flock --close 100 die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")" fi -# do not let pacman run while we do this -touch "$lockfile"

workdir=$(mktemp -d "${TMPDIR:-/tmp}/pacman-optimize.XXXXXXXXXX") || die_r "$(gettext "Can not create temp directory for database building.")\n" >&2 @@ -178,6 +178,7 @@ rm -rf "$localdb.old" # remove the lock file and our working directory with sums and tarfile rm -f "$lockfile" +flock --close 100 rm -rf "$workdir"

echo -- 1.8.3.4

On Aug 14, 2013 8:38 PM, "Mattias Andrée" wrote:

I understood that. But I did not know that pacman supports BSD or Max OS X, nor that it was not portable. Just leaving it out because of NFS seems silly, it is not up to the program to account for all software that does not implement expected standards.

Uhhh but it is. That's sort of the idea of software portability. And since when is file locking support anything close to standardized? It's incredibly OS and filesystem specific.

On Wed, 14 Aug 2013 19:29:41 -0400 Dave Reisner wrote:

...

On Aug 14, 2013 7:21 PM, "Mattias Andrée" wrote:

...

That is ridiculus that NFS does not implement flock, but there is a solution documented in flock(2):

flock() does not lock files over NFS. Use fcntl(2) instead: that does work over NFS, given a sufficiently recent version of Linux and a server which supports locking.

Keyword being Linux. We support BSD and OSX as well. fcntl semantics aren't portable.

Again, you're not the first to go down this road.

...

On Wed, 14 Aug 2013 18:14:20 -0400 Dave Reisner wrote:

...

On Thu, Aug 15, 2013 at 12:03:33AM +0200, Mattias Andrée wrote:

...

This allows for further improvement of pacman where shared locks could also be used, in cases there pacman only reads the database and does not edit it.

File locks also makes it easy to let pacman wait until the block instance has finished, by just not using the LOCK_NB flag. ---

And the question that comes up every time this patch arises: what about NFS?

No, we won't change this.

...

lib/libalpm/be_sync.c | 5 +---- lib/libalpm/handle.c | 26 ++++++++++++++------------ lib/libalpm/handle.h | 2 +- lib/libalpm/trans.c | 7 +------ scripts/pacman-db-upgrade.sh.in | 9 +++++---- scripts/pacman-optimize.sh.in | 9 +++++---- 6 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 0b99684..e7c8c4b 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -284,10 +284,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) cleanup:

- if(_alpm_handle_unlock(handle)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("could not remove lock file %s\n"), - handle->lockfile); - } + _alpm_handle_unlock(handle); free(syncpath); umask(oldmask); return ret; diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 53c86c5..5d1cfb7 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -27,6 +27,7 @@ #include #include #include +#include #include

/* libalpm */ @@ -44,6 +45,7 @@ alpm_handle_t *_alpm_handle_new(void)

CALLOC(handle, 1, sizeof(alpm_handle_t), return NULL); handle->deltaratio = 0.0; + handle->lckstream = -1;

return handle; } @@ -95,7 +97,7 @@ int _alpm_handle_lock(alpm_handle_t *handle) char *dir, *ptr;

ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream == NULL, return 0); + ASSERT(handle->lckstream < 0, return 0);

/* create the dir of the lockfile first */ dir = strdup(handle->lockfile); @@ -110,14 +112,14 @@ int _alpm_handle_lock(alpm_handle_t *handle) FREE(dir);

do { - fd = open(handle->lockfile, O_WRONLY | O_CREAT | O_EXCL, 0000); + fd = open(handle->lockfile, O_WRONLY | O_CREAT); } while(fd == -1 && errno == EINTR); if(fd >= 0) { - FILE *f = fdopen(fd, "w"); - fprintf(f, "%ld\n", (long)getpid()); - fflush(f); fsync(fd); - handle->lckstream = f; + if(flock(fd, LOCK_EX | LOCK_NB) < 0) { + return -1; + } + handle->lckstream = fd; return 0; } return -1; @@ -127,14 +129,14 @@ int _alpm_handle_lock(alpm_handle_t *handle) int _alpm_handle_unlock(alpm_handle_t *handle) { ASSERT(handle->lockfile != NULL, return -1); - ASSERT(handle->lckstream != NULL, return 0); - - fclose(handle->lckstream); - handle->lckstream = NULL; + ASSERT(handle->lckstream >= 0, return 0);

- if(unlink(handle->lockfile) && errno != ENOENT) { - return -1; + flock(handle->lckstream, LOCK_UN); + if(flock(handle->lckstream, LOCK_EX | LOCK_NB) == 0) { + unlink(handle->lockfile); + flock(handle->lckstream, LOCK_UN); } + handle->lckstream = NULL; return 0; }

diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5e84d58..7ca90ff 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -55,7 +55,7 @@ struct __alpm_handle_t { alpm_db_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (alpm_db_t *) */ FILE *logstream; /* log file stream pointer */ - FILE *lckstream; /* lock file stream pointer if one exists */ + int lckstream; /* lock file file descriptor if one exists */ alpm_trans_t *trans;

#ifdef HAVE_LIBCURL diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 8d4e0e7..122f35b 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -215,12 +215,7 @@ int SYMEXPORT alpm_trans_release(alpm_handle_t *handle) /* unlock db */ if(!nolock_flag) { - if(_alpm_handle_unlock(handle)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("could not remove lock file %s\n"), - handle->lockfile); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: could not remove lock file %s\n", handle->lockfile); - } + _alpm_handle_unlock(handle); }

return 0; diff --git a/scripts/pacman-db-upgrade.sh.in b/scripts/pacman-db-upgrade.sh.in index a1630c5..20cf035 100644 --- a/scripts/pacman-db-upgrade.sh.in +++ b/scripts/pacman-db-upgrade.sh.in @@ -102,12 +102,12 @@ dbroot="${dbroot%/}" # form the path to our lockfile location lockfile="${dbroot}/db.lck"

-# make sure pacman isn't running -if [[ -f $lockfile ]]; then +# make sure pacman isn't running and stop other instances for starting +exec 100<>"$lockfile" +if ! flock -x --nonblock 100; then + flock --close 100 die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")" fi -# do not let pacman run while we do this -touch "$lockfile"

# pacman-3.4 to 3.5 upgrade - merge depends into desc if [[ $(find "$dbroot"/local -name depends) ]]; then @@ -123,5 +123,6 @@ fi

# remove the lock file rm -f "$lockfile" +flock --close 100

# vim: set ts=2 sw=2 noet: diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in index 47fbb49..8c6e556 100644 --- a/scripts/pacman-optimize.sh.in +++ b/scripts/pacman-optimize.sh.in @@ -114,12 +114,12 @@ dbroot="${dbroot%/}" lockfile="${dbroot}/db.lck" localdb="${dbroot}/local"

-# make sure pacman isn't running -if [[ -f $lockfile ]]; then +# make sure pacman isn't running and stop other instances for starting +exec 100<>"$lockfile" +if ! flock -x --nonblock 100; then + flock --close 100 die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")" fi -# do not let pacman run while we do this -touch "$lockfile"

workdir=$(mktemp -d "${TMPDIR:-/tmp}/pacman-optimize.XXXXXXXXXX") || die_r "$(gettext "Can not create temp directory for database building.")\n" >&2 @@ -178,6 +178,7 @@ rm -rf "$localdb.old" # remove the lock file and our working directory with sums and tarfile rm -f "$lockfile" +flock --close 100 rm -rf "$workdir"

echo -- 1.8.3.4