[PATCH] flock: add support for using fcntl() with open file description locks

[PATCH] flock: add support for using fcntl() with open file description locks

[Rasmus Villemoes]

Currently, there is no way for shell scripts to safely access
resources protected by POSIX locking (fcntl with the F_SETLK/F_SETLKW
commands). For example, the glibc function lckpwdf(), used to
protect access to the /etc/shadow database, works by taking a
F_SETLKW on /etc/.pwd.lock .

Due to the odd semantics of POSIX locking (e.g. released when any file
descriptor associated to the inode is closed), we cannot usefully
directly expose the POSIX F_SETLK/F_SETLKW commands. However, linux
3.15 introduced F_OFD_SETLK[W], with semantics wrt. ownership and
release better matching those of flock(2), and crucially they do
conflict with locks obtained via F_SETLK[W]. With this, a shell script
can do

  exec 4> /etc/.pwd.lock
  flock --fcntl-ofd 4
  <access/modify /etc/shadow ...>
  flock --fcntl-ofd --unlock 4 # or just exit

without conflicting with passwd(1) or other utilities that
access/modify /etc/shadow.

The option name is a bit verbose, and no single-letter shorthand is
defined, because this is somewhat low-level and the user really needs
to know what he is doing.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

---

Both my autotools and meson fu are weak to non-existing, so I don't
know if I've written the "test if the header exposes this macro"
correctly.

I'm not at all married to the option name. I also considered just
making it --fcntl, with the possiblity of making that grow an optional
argument (for example --fcntl=posix with plain --fcntl being an alias
for --fcntl=ofd) should anyone ever figure out a use for the plain
F_SETLK, perhaps just for testing.


 configure.ac      |  6 +++++
 meson.build       |  3 +++
 sys-utils/flock.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index c302732e7..441b09440 100644
--- a/configure.ac
+++ b/configure.ac
@@ -434,6 +434,12 @@ AC_CHECK_DECLS([PR_REP_CAPACITY], [], [], [
 	#include <linux/pr.h>
 ])
 
+AC_CHECK_DECL([F_OFD_SETLK],
+	[AC_DEFINE([HAVE_FCNTL_OFD_LOCKS], [1],
+	[Define to 1 if fcntl.h defines F_OFD_ constants])], [], [
+#include <fcntl.h>
+])
+
 AC_CHECK_HEADERS([security/openpam.h], [], [], [
 #ifdef HAVE_SECURITY_PAM_APPL_H
 #include <security/pam_appl.h>
diff --git a/meson.build b/meson.build
index 99126f7aa..004c849f1 100644
--- a/meson.build
+++ b/meson.build
@@ -704,6 +704,9 @@ conf.set('HAVE_DECL_BLK_ZONE_REP_CAPACITY', have ? 1 : false)
 have = cc.has_header_symbol('linux/pr.h', 'PR_REP_CAPACITY')
 conf.set('HAVE_DECL_PR_REP_CAPACITY', have ? 1 : false)
 
+have = cc.has_header_symbol('fcntl.h', 'F_OFD_SETLK', args: '-D_GNU_SOURCE')
+conf.set('HAVE_FCNTL_OFD_LOCKS', have ? 1 : false)
+
 code = '''
 #include <time.h>
 #if !@0@
diff --git a/sys-utils/flock.c b/sys-utils/flock.c
index 7d878ff81..40751517d 100644
--- a/sys-utils/flock.c
+++ b/sys-utils/flock.c
@@ -70,6 +70,9 @@ static void __attribute__((__noreturn__)) usage(void)
 	fputs(_(  " -o, --close              close file descriptor before running command\n"), stdout);
 	fputs(_(  " -c, --command <command>  run a single command string through the shell\n"), stdout);
 	fputs(_(  " -F, --no-fork            execute command without forking\n"), stdout);
+#ifdef HAVE_FCNTL_OFD_LOCKS
+	fputs(_(  "     --fcntl-ofd          use fcntl(F_OFD_SETLK) rather than flock()\n"), stdout);
+#endif
 	fputs(_(  "     --verbose            increase verbosity\n"), stdout);
 	fputs(USAGE_SEPARATOR, stdout);
 	fprintf(stdout, USAGE_HELP_OPTIONS(26));
@@ -126,6 +129,38 @@ static void __attribute__((__noreturn__)) run_program(char **cmd_argv)
 	_exit((errno == ENOMEM) ? EX_OSERR : EX_UNAVAILABLE);
 }
 
+static int flock_to_fcntl_type(int op)
+{
+        switch (op) {
+                case LOCK_EX:
+                        return F_WRLCK;
+                case LOCK_SH:
+                        return F_RDLCK;
+                case LOCK_UN:
+                        return F_UNLCK;
+                default:
+			errx(EX_SOFTWARE, _("internal error, unknown operation %d"), op);
+        }
+}
+
+static int do_fcntl_lock(int fd, int op, int block)
+{
+#ifdef HAVE_FCNTL_OFD_LOCKS
+	struct flock arg = {
+		.l_type = flock_to_fcntl_type(op),
+		.l_whence = SEEK_SET,
+		.l_start = 0,
+		.l_len = 0,
+	};
+	int cmd = (block == LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW;
+	return fcntl(fd, cmd, &arg);
+#else
+	/* Should never happen, nothing can ever set use_fcntl_ofd when !HAVE_FCNTL_OFD_LOCKS. */
+	errno = ENOSYS;
+	return -1;
+#endif
+}
+
 int main(int argc, char *argv[])
 {
 	struct ul_timer timer;
@@ -140,6 +175,7 @@ int main(int argc, char *argv[])
 	int no_fork = 0;
 	int status;
 	int verbose = 0;
+	int use_fcntl_ofd = 0;
 	struct timeval time_start = { 0 }, time_done = { 0 };
 	/*
 	 * The default exit code for lock conflict or timeout
@@ -149,7 +185,8 @@ int main(int argc, char *argv[])
 	char **cmd_argv = NULL, *sh_c_argv[4];
 	const char *filename = NULL;
 	enum {
-		OPT_VERBOSE = CHAR_MAX + 1
+		OPT_VERBOSE = CHAR_MAX + 1,
+		OPT_FCNTL_OFD,
 	};
 	static const struct option long_options[] = {
 		{"shared", no_argument, NULL, 's'},
@@ -163,6 +200,7 @@ int main(int argc, char *argv[])
 		{"close", no_argument, NULL, 'o'},
 		{"no-fork", no_argument, NULL, 'F'},
 		{"verbose", no_argument, NULL, OPT_VERBOSE},
+		{"fcntl-ofd", no_argument, NULL, OPT_FCNTL_OFD},
 		{"help", no_argument, NULL, 'h'},
 		{"version", no_argument, NULL, 'V'},
 		{NULL, 0, NULL, 0}
@@ -217,6 +255,11 @@ int main(int argc, char *argv[])
 			if (conflict_exit_code < 0 || conflict_exit_code > 255)
 				errx(EX_USAGE, _("exit code out of range (expected 0 to 255)"));
 			break;
+#ifdef HAVE_FCNTL_OFD_LOCKS
+		case OPT_FCNTL_OFD:
+			use_fcntl_ofd = 1;
+			break;
+#endif
 		case OPT_VERBOSE:
 			verbose = 1;
 			break;
@@ -234,6 +277,13 @@ int main(int argc, char *argv[])
 		errx(EX_USAGE,
 			_("the --no-fork and --close options are incompatible"));
 
+	/*
+	 * For fcntl(F_OFD_SETLK), an exclusive lock requires that the
+	 * file is open for write.
+	 */
+	if (use_fcntl_ofd && type == LOCK_EX)
+		open_flags = O_WRONLY;
+
 	if (argc > optind + 1) {
 		/* Run command */
 		if (!strcmp(argv[optind + 1], "-c") ||
@@ -280,9 +330,15 @@ int main(int argc, char *argv[])
 
 	if (verbose)
 		gettime_monotonic(&time_start);
-	while (flock(fd, type | block)) {
+	while (use_fcntl_ofd ? do_fcntl_lock(fd, type, block) : flock(fd, type | block)) {
 		switch (errno) {
 		case EWOULDBLOCK:
+			/*
+			 * Per the man page, for fcntl(), EACCES may
+			 * be returned and means the same as
+			 * EAGAIN/EWOULDBLOCK.
+			 */
+		case EACCES:
 			/* -n option set and failed to lock. */
 			if (verbose)
 				warnx(_("failed to get lock"));
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH] flock: add support for using fcntl() with open file description locks

[Masatake YAMATO]

> Currently, there is no way for shell scripts to safely access
> resources protected by POSIX locking (fcntl with the F_SETLK/F_SETLKW
> commands). For example, the glibc function lckpwdf(), used to
> protect access to the /etc/shadow database, works by taking a
> F_SETLKW on /etc/.pwd.lock .
> 
> Due to the odd semantics of POSIX locking (e.g. released when any file
> descriptor associated to the inode is closed), we cannot usefully
> directly expose the POSIX F_SETLK/F_SETLKW commands. However, linux
> 3.15 introduced F_OFD_SETLK[W], with semantics wrt. ownership and
> release better matching those of flock(2), and crucially they do
> conflict with locks obtained via F_SETLK[W]. With this, a shell script
> can do
> 
>   exec 4> /etc/.pwd.lock
>   flock --fcntl-ofd 4
>   <access/modify /etc/shadow ...>
>   flock --fcntl-ofd --unlock 4 # or just exit
> 
> without conflicting with passwd(1) or other utilities that
> access/modify /etc/shadow.
> 
> The option name is a bit verbose, and no single-letter shorthand is
> defined, because this is somewhat low-level and the user really needs
> to know what he is doing.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> 
> ---
> 
> Both my autotools and meson fu are weak to non-existing, so I don't
> know if I've written the "test if the header exposes this macro"
> correctly.
> 
> I'm not at all married to the option name. I also considered just
> making it --fcntl, with the possiblity of making that grow an optional
> argument (for example --fcntl=posix with plain --fcntl being an alias
> for --fcntl=ofd) should anyone ever figure out a use for the plain
> F_SETLK, perhaps just for testing.
> 
> 
>  configure.ac      |  6 +++++
>  meson.build       |  3 +++
>  sys-utils/flock.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 67 insertions(+), 2 deletions(-)

You may have to update sys-utils/flock.1.adoc and the completion rule bash-completion/flock
when adding a new optoin.

> diff --git a/configure.ac b/configure.ac
> index c302732e7..441b09440 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -434,6 +434,12 @@ AC_CHECK_DECLS([PR_REP_CAPACITY], [], [], [
>  	#include <linux/pr.h>
>  ])
>  
> +AC_CHECK_DECL([F_OFD_SETLK],
> +	[AC_DEFINE([HAVE_FCNTL_OFD_LOCKS], [1],
> +	[Define to 1 if fcntl.h defines F_OFD_ constants])], [], [
> +#include <fcntl.h>
> +])
> +
>  AC_CHECK_HEADERS([security/openpam.h], [], [], [
>  #ifdef HAVE_SECURITY_PAM_APPL_H
>  #include <security/pam_appl.h>
> diff --git a/meson.build b/meson.build
> index 99126f7aa..004c849f1 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -704,6 +704,9 @@ conf.set('HAVE_DECL_BLK_ZONE_REP_CAPACITY', have ? 1 : false)
>  have = cc.has_header_symbol('linux/pr.h', 'PR_REP_CAPACITY')
>  conf.set('HAVE_DECL_PR_REP_CAPACITY', have ? 1 : false)
>  
> +have = cc.has_header_symbol('fcntl.h', 'F_OFD_SETLK', args: '-D_GNU_SOURCE')
> +conf.set('HAVE_FCNTL_OFD_LOCKS', have ? 1 : false)
> +
>  code = '''
>  #include <time.h>
>  #if !@0@
> diff --git a/sys-utils/flock.c b/sys-utils/flock.c
> index 7d878ff81..40751517d 100644
> --- a/sys-utils/flock.c
> +++ b/sys-utils/flock.c
> @@ -70,6 +70,9 @@ static void __attribute__((__noreturn__)) usage(void)
>  	fputs(_(  " -o, --close              close file descriptor before running command\n"), stdout);
>  	fputs(_(  " -c, --command <command>  run a single command string through the shell\n"), stdout);
>  	fputs(_(  " -F, --no-fork            execute command without forking\n"), stdout);
> +#ifdef HAVE_FCNTL_OFD_LOCKS
> +	fputs(_(  "     --fcntl-ofd          use fcntl(F_OFD_SETLK) rather than flock()\n"), stdout);
> +#endif
>  	fputs(_(  "     --verbose            increase verbosity\n"), stdout);
>  	fputs(USAGE_SEPARATOR, stdout);
>  	fprintf(stdout, USAGE_HELP_OPTIONS(26));
> @@ -126,6 +129,38 @@ static void __attribute__((__noreturn__)) run_program(char **cmd_argv)
>  	_exit((errno == ENOMEM) ? EX_OSERR : EX_UNAVAILABLE);
>  }
>  
> +static int flock_to_fcntl_type(int op)
> +{
> +        switch (op) {
> +                case LOCK_EX:
> +                        return F_WRLCK;
> +                case LOCK_SH:
> +                        return F_RDLCK;
> +                case LOCK_UN:
> +                        return F_UNLCK;
> +                default:
> +			errx(EX_SOFTWARE, _("internal error, unknown operation %d"), op);
> +        }
> +}

Don't you need wrap flock_to_fcntl_type with #ifdef HAVE_FCNTL_OFD_LOCKS/#endif?

> +static int do_fcntl_lock(int fd, int op, int block)
> +{
> +#ifdef HAVE_FCNTL_OFD_LOCKS
> +	struct flock arg = {
> +		.l_type = flock_to_fcntl_type(op),
> +		.l_whence = SEEK_SET,
> +		.l_start = 0,
> +		.l_len = 0,
> +	};
> +	int cmd = (block == LOCK_NB) ? F_OFD_SETLK : F_OFD_SETLKW;
> +	return fcntl(fd, cmd, &arg);
> +#else
> +	/* Should never happen, nothing can ever set use_fcntl_ofd when !HAVE_FCNTL_OFD_LOCKS. */
> +	errno = ENOSYS;
> +	return -1;
> +#endif
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct ul_timer timer;
> @@ -140,6 +175,7 @@ int main(int argc, char *argv[])
>  	int no_fork = 0;
>  	int status;
>  	int verbose = 0;
> +	int use_fcntl_ofd = 0;
>  	struct timeval time_start = { 0 }, time_done = { 0 };
>  	/*
>  	 * The default exit code for lock conflict or timeout
> @@ -149,7 +185,8 @@ int main(int argc, char *argv[])
>  	char **cmd_argv = NULL, *sh_c_argv[4];
>  	const char *filename = NULL;
>  	enum {
> -		OPT_VERBOSE = CHAR_MAX + 1
> +		OPT_VERBOSE = CHAR_MAX + 1,
> +		OPT_FCNTL_OFD,
>  	};
>  	static const struct option long_options[] = {
>  		{"shared", no_argument, NULL, 's'},
> @@ -163,6 +200,7 @@ int main(int argc, char *argv[])
>  		{"close", no_argument, NULL, 'o'},
>  		{"no-fork", no_argument, NULL, 'F'},
>  		{"verbose", no_argument, NULL, OPT_VERBOSE},
> +		{"fcntl-ofd", no_argument, NULL, OPT_FCNTL_OFD},
>  		{"help", no_argument, NULL, 'h'},
>  		{"version", no_argument, NULL, 'V'},
>  		{NULL, 0, NULL, 0}
> @@ -217,6 +255,11 @@ int main(int argc, char *argv[])
>  			if (conflict_exit_code < 0 || conflict_exit_code > 255)
>  				errx(EX_USAGE, _("exit code out of range (expected 0 to 255)"));
>  			break;
> +#ifdef HAVE_FCNTL_OFD_LOCKS
> +		case OPT_FCNTL_OFD:
> +			use_fcntl_ofd = 1;
> +			break;
> +#endif
>  		case OPT_VERBOSE:
>  			verbose = 1;
>  			break;
> @@ -234,6 +277,13 @@ int main(int argc, char *argv[])
>  		errx(EX_USAGE,
>  			_("the --no-fork and --close options are incompatible"));
>  
> +	/*
> +	 * For fcntl(F_OFD_SETLK), an exclusive lock requires that the
> +	 * file is open for write.
> +	 */
> +	if (use_fcntl_ofd && type == LOCK_EX)
> +		open_flags = O_WRONLY;
> +
>  	if (argc > optind + 1) {
>  		/* Run command */
>  		if (!strcmp(argv[optind + 1], "-c") ||
> @@ -280,9 +330,15 @@ int main(int argc, char *argv[])
>  
>  	if (verbose)
>  		gettime_monotonic(&time_start);
> -	while (flock(fd, type | block)) {
> +	while (use_fcntl_ofd ? do_fcntl_lock(fd, type, block) : flock(fd, type | block)) {
>  		switch (errno) {
>  		case EWOULDBLOCK:
> +			/*
> +			 * Per the man page, for fcntl(), EACCES may
> +			 * be returned and means the same as
> +			 * EAGAIN/EWOULDBLOCK.
> +			 */
> +		case EACCES:
>  			/* -n option set and failed to lock. */
>  			if (verbose)
>  				warnx(_("failed to get lock"));
> -- 
> 2.40.1.1.g1c60b9335d
> 
> 

Masatake YAMATO

Re: [PATCH] flock: add support for using fcntl() with open file description locks

[Rasmus Villemoes]

On 17/04/2024 19.33, Masatake YAMATO wrote:
>> Currently, there is no way for shell scripts to safely access
>> resources protected by POSIX locking (fcntl with the F_SETLK/F_SETLKW
>> commands). For example, the glibc function lckpwdf(), used to
>> protect access to the /etc/shadow database, works by taking a
>> F_SETLKW on /etc/.pwd.lock .
>>
>> Due to the odd semantics of POSIX locking (e.g. released when any file
>> descriptor associated to the inode is closed), we cannot usefully
>> directly expose the POSIX F_SETLK/F_SETLKW commands. However, linux
>> 3.15 introduced F_OFD_SETLK[W], with semantics wrt. ownership and
>> release better matching those of flock(2), and crucially they do
>> conflict with locks obtained via F_SETLK[W]. With this, a shell script
>> can do
>>
>>   exec 4> /etc/.pwd.lock
>>   flock --fcntl-ofd 4
>>   <access/modify /etc/shadow ...>
>>   flock --fcntl-ofd --unlock 4 # or just exit
>>
>> without conflicting with passwd(1) or other utilities that
>> access/modify /etc/shadow.
>>
>> The option name is a bit verbose, and no single-letter shorthand is
>> defined, because this is somewhat low-level and the user really needs
>> to know what he is doing.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>
>> ---
>>
>> Both my autotools and meson fu are weak to non-existing, so I don't
>> know if I've written the "test if the header exposes this macro"
>> correctly.
>>
>> I'm not at all married to the option name. I also considered just
>> making it --fcntl, with the possiblity of making that grow an optional
>> argument (for example --fcntl=posix with plain --fcntl being an alias
>> for --fcntl=ofd) should anyone ever figure out a use for the plain
>> F_SETLK, perhaps just for testing.
>>
>>
>>  configure.ac      |  6 +++++
>>  meson.build       |  3 +++
>>  sys-utils/flock.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
>>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> You may have to update sys-utils/flock.1.adoc and the completion rule bash-completion/flock
> when adding a new optoin.

I will send a separate patch with that once the option naming is settled
and the concept itself is accepted (not necessarily applied to master).

>>  code = '''
>>  #include <time.h>
>>  #if !@0@
>> diff --git a/sys-utils/flock.c b/sys-utils/flock.c
>> index 7d878ff81..40751517d 100644
>> --- a/sys-utils/flock.c
>> +++ b/sys-utils/flock.c
>> @@ -70,6 +70,9 @@ static void __attribute__((__noreturn__)) usage(void)
>>  	fputs(_(  " -o, --close              close file descriptor before running command\n"), stdout);
>>  	fputs(_(  " -c, --command <command>  run a single command string through the shell\n"), stdout);
>>  	fputs(_(  " -F, --no-fork            execute command without forking\n"), stdout);
>> +#ifdef HAVE_FCNTL_OFD_LOCKS
>> +	fputs(_(  "     --fcntl-ofd          use fcntl(F_OFD_SETLK) rather than flock()\n"), stdout);
>> +#endif
>>  	fputs(_(  "     --verbose            increase verbosity\n"), stdout);
>>  	fputs(USAGE_SEPARATOR, stdout);
>>  	fprintf(stdout, USAGE_HELP_OPTIONS(26));
>> @@ -126,6 +129,38 @@ static void __attribute__((__noreturn__)) run_program(char **cmd_argv)
>>  	_exit((errno == ENOMEM) ? EX_OSERR : EX_UNAVAILABLE);
>>  }
>>  
>> +static int flock_to_fcntl_type(int op)
>> +{
>> +        switch (op) {
>> +                case LOCK_EX:
>> +                        return F_WRLCK;
>> +                case LOCK_SH:
>> +                        return F_RDLCK;
>> +                case LOCK_UN:
>> +                        return F_UNLCK;
>> +                default:
>> +			errx(EX_SOFTWARE, _("internal error, unknown operation %d"), op);
>> +        }
>> +}
> 
> Don't you need wrap flock_to_fcntl_type with #ifdef HAVE_FCNTL_OFD_LOCKS/#endif?

Well, the constants mentioned here are the same as used with posix
locking, and have been in glibc and kernel headers since forever. Only
the F_OFD_* ones are "newish". So I don't think this needs guarding due
to non-existence of the symbols. But I might need to guard the whole
function (or mark it maybe-unused) to avoid a defined-but-not-used warning.

I wasn't even sure if I actually needed a configure check and HAVE_*
guard at all; 3.15 is 10 years old by now, but I couldn't find any
mention of the oldest glibc/kernel that util-linux is supposed to build
against. Karel?

Rasmus

Re: [PATCH] flock: add support for using fcntl() with open file description locks

[Rasmus Villemoes]

On 17/04/2024 12.09, Rasmus Villemoes wrote:

> I'm not at all married to the option name. I also considered just
> making it --fcntl, 

Thinking more about it, I think I prefer this slightly shorter name.

with the possiblity of making that grow an optional
> argument (for example --fcntl=posix with plain --fcntl being an alias
> for --fcntl=ofd) 

Not that I'm gonna implement this part immediately, but I do like that
it leaves the door open for it.

If no other comments appear in a day or two, I'll resend with that
change and the fix for guarding the maybe-unused helper function.

Rasmus

Re: [PATCH] flock: add support for using fcntl() with open file description locks

[Karel Zak]

On Thu, Apr 18, 2024 at 02:33:54AM +0900, Masatake YAMATO wrote:
> >  int main(int argc, char *argv[])
> >  {
> >  	struct ul_timer timer;
> > @@ -140,6 +175,7 @@ int main(int argc, char *argv[])
> >  	int no_fork = 0;
> >  	int status;
> >  	int verbose = 0;
> > +	int use_fcntl_ofd = 0;

What about to introduce "lock_method" variable and define

 enum {
    LOCK_BY_FLOCK
    LOCK_BY_FCNTL_OFD
 };

later you can easily extend it by LOCK_BY_FCNTL_POSIX. IMHO the code
will be more readable.

> >  	struct timeval time_start = { 0 }, time_done = { 0 };
> >  	/*
> >  	 * The default exit code for lock conflict or timeout
> > @@ -149,7 +185,8 @@ int main(int argc, char *argv[])
> >  	char **cmd_argv = NULL, *sh_c_argv[4];
> >  	const char *filename = NULL;
> >  	enum {
> > -		OPT_VERBOSE = CHAR_MAX + 1
> > +		OPT_VERBOSE = CHAR_MAX + 1,
> > +		OPT_FCNTL_OFD,
> >  	};
> >  	static const struct option long_options[] = {
> >  		{"shared", no_argument, NULL, 's'},
> > @@ -163,6 +200,7 @@ int main(int argc, char *argv[])
> >  		{"close", no_argument, NULL, 'o'},
> >  		{"no-fork", no_argument, NULL, 'F'},
> >  		{"verbose", no_argument, NULL, OPT_VERBOSE},
> > +		{"fcntl-ofd", no_argument, NULL, OPT_FCNTL_OFD},

I agree that the sort name --fcntl sounds better and it's extendable.

> >  		{"help", no_argument, NULL, 'h'},
> >  		{"version", no_argument, NULL, 'V'},
> >  		{NULL, 0, NULL, 0}
> > @@ -217,6 +255,11 @@ int main(int argc, char *argv[])
> >  			if (conflict_exit_code < 0 || conflict_exit_code > 255)
> >  				errx(EX_USAGE, _("exit code out of range (expected 0 to 255)"));
> >  			break;
> > +#ifdef HAVE_FCNTL_OFD_LOCKS
> > +		case OPT_FCNTL_OFD:
> > +			use_fcntl_ofd = 1;
> > +			break;
> > +#endif
> >  		case OPT_VERBOSE:
> >  			verbose = 1;
> >  			break;
> > @@ -234,6 +277,13 @@ int main(int argc, char *argv[])
> >  		errx(EX_USAGE,
> >  			_("the --no-fork and --close options are incompatible"));
> >  
> > +	/*
> > +	 * For fcntl(F_OFD_SETLK), an exclusive lock requires that the
> > +	 * file is open for write.
> > +	 */
> > +	if (use_fcntl_ofd && type == LOCK_EX)
> > +		open_flags = O_WRONLY;
> > +
> >  	if (argc > optind + 1) {
> >  		/* Run command */
> >  		if (!strcmp(argv[optind + 1], "-c") ||
> > @@ -280,9 +330,15 @@ int main(int argc, char *argv[])
> >  
> >  	if (verbose)
> >  		gettime_monotonic(&time_start);
> > -	while (flock(fd, type | block)) {
> > +	while (use_fcntl_ofd ? do_fcntl_lock(fd, type, block) : flock(fd, type | block)) {

Maybe we can move the locking to a function

    do_lock(fd, method, type, block)

and hide the necessary flock and fcntl details there, instead of
trying to do it in main().

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] flock: add support for using fcntl() with open file description locks

[Karel Zak]

On Tue, Apr 23, 2024 at 11:50:05AM +0200, Karel Zak wrote:
> On Thu, Apr 18, 2024 at 02:33:54AM +0900, Masatake YAMATO wrote:

Oh, sorry, I replied to Masatake's email instead of Rasmus's. My notes
are for the original patch :-)

    Karel
-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com