Re: [OE-core] [pseudo][PATCH v2] fcntl: Add support for fcntl F_GETPIPE_SZ and F_SETPIPE_SZ

Re: [OE-core] [pseudo][PATCH v2] fcntl: Add support for fcntl F_GETPIPE_SZ and F_SETPIPE_SZ

[Mike Crowe]

On Friday 13 August 2021 at 12:05:09 +0100, Mike Crowe via lists.openembedded.org wrote:
> When running the test suite on my Debian 11 box I see many occurrences
> of:
> 
>  unknown fcntl argument 1032, assuming long argument.
> 
> (for example from test-execl.sh.)
> 
> It appears that this is F_GETPIPE_SZ and it takes no arguments. Let's
> add it and the corresponding F_SETPIPE_SZ too to avoid the warning
> messages.
> 
> F_SETPIPE_SZ accepts an int argument, which strictly speaking isn't the
> same as the long that the wrapper expects. However, this is also true
> for F_DUPFD which seems to be working correctly on all the targets that
> people care about.
> 
> We need to define the command constants if the system headers don't
> provide them to ensure that a binary built on an old system works
> without the new commands works correctly only a newer one that tries to
> use them. If the system values differ from the expected ones then such a
> binary would also be incompatible, so fail the build in that case too.
> 
> Signed-off-by: Mike Crowe <mac@mcrowe.com>
> ---
>  Makefile.in              |  1 +
>  ports/linux/guts/fcntl.c | 21 +++++++++++++++
>  test/test-fcntl.c        | 58 ++++++++++++++++++++++++++++++++++++++++
>  test/test-fcntl.sh       |  5 ++++
>  4 files changed, 85 insertions(+)
>  create mode 100644 test/test-fcntl.c
>  create mode 100755 test/test-fcntl.sh
> 
> diff --git a/Makefile.in b/Makefile.in
> index 10441ef..4ebe5da 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -79,6 +79,7 @@ test: all | $(BIN) $(LIB) $(LOCALSTATE)
>  	$(CC) $(CFLAGS) $(CFLAGS_PSEUDO) -o test/test-openat test/test-openat.c
>  	$(CC) $(CFLAGS) $(CFLAGS_PSEUDO) -o test/test-statx test/test-statx.c
>  	$(CC) $(CFLAGS) $(CFLAGS_PSEUDO) -o test/test-fstat test/test-fstat.c
> +	$(CC) $(CFLAGS) $(CFLAGS_PSEUDO) -o test/test-fcntl test/test-fcntl.c
>  	@./run_tests.sh -v
>  
>  install-lib: $(LIBPSEUDO)
> diff --git a/ports/linux/guts/fcntl.c b/ports/linux/guts/fcntl.c
> index 434c7f3..ffb50be 100644
> --- a/ports/linux/guts/fcntl.c
> +++ b/ports/linux/guts/fcntl.c
> @@ -8,6 +8,22 @@
>   * wrap_fcntl(int fd, int cmd, ...struct flock *lock) {
>   *	int rc = -1;
>   */
> +#if !defined(F_GETPIPE_SZ)
> +#define F_GETPIPE_SZ (1032)
> +#endif
> +
> +#if F_GETPIPE_SZ != 1032
> +#error System F_GETPIPE_SZ has unexpected value
> +#endif
> +
> +#if !defined(F_SETPIPE_SZ)
> +#define F_SETPIPE_SZ (1031)
> +#endif
> +
> +#if F_SETPIPE_SZ != 1031
> +#error System F_SETPIPE_SZ has unexpected value
> +#endif
> +
>   	long arg;
>  	int save_errno;
>  
> @@ -31,12 +47,17 @@
>  		}
>  		errno = save_errno;
>  		break;
> +	case F_SETPIPE_SZ:
> +		/* actually do something */
> +		rc = real_fcntl(fd, cmd, arg);
> +		break;
>  	/* no argument: */
>  	case F_GETFD:
>  	case F_GETFL:
>  	case F_GETOWN:
>  	case F_GETSIG:
>  	case F_GETLEASE:
> +	case F_GETPIPE_SZ:
>  		rc = real_fcntl(fd, cmd);
>  		break;
>  	/* long argument */
> diff --git a/test/test-fcntl.c b/test/test-fcntl.c
> new file mode 100644
> index 0000000..b593d50
> --- /dev/null
> +++ b/test/test-fcntl.c
> @@ -0,0 +1,58 @@
> +/* fcntl-linux.h doesn't define F_GETPIPE_SZ and F_SETPIPE_SZ without
> + * this */
> +#define _GNU_SOURCE
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <unistd.h>
> +#include <string.h>
> +
> +int test_pipe_sz()
> +{
> +#if defined(F_GETPIPE_SZ) && defined(F_SETPIPE_SZ)
> +	int pipefd[2];
> +
> +	if (pipe(pipefd) < 0) {
> +		perror("pipe");
> +		return 1;
> +	}
> +
> +	const int orig_size = fcntl(pipefd[0], F_GETPIPE_SZ);
> +	if (orig_size < 0) {
> +		perror("F_GETPIPE_SZ");
> +		return 1;
> +	}
> +
> +	const int new_size = orig_size * 2;
> +
> +	if (fcntl(pipefd[0], F_SETPIPE_SZ, new_size) < 0) {
> +		perror("F_SETPIPE_SZ");
> +		return 1;
> +	}
> +
> +	const int final_size = fcntl(pipefd[0], F_GETPIPE_SZ);
> +	if (final_size < 0) {
> +		perror("Second F_GETPIPE_SZ");
> +		return 1;
> +	}
> +
> +	if (final_size < new_size) {
> +		fprintf(stderr, "Unexpected final pipe size: %d\n", final_size);
> +		return 1;
> +	}
> +#else
> +	printf("Host too old for F_GETPIPE_SZ and F_SETPIPE_SZ tests\n");
> +#endif
> +	return 0;
> +}
> +
> +int main()
> +{
> +	int result = 0;
> +	result += test_pipe_sz();
> +	return result;
> +}
> diff --git a/test/test-fcntl.sh b/test/test-fcntl.sh
> new file mode 100755
> index 0000000..7112620
> --- /dev/null
> +++ b/test/test-fcntl.sh
> @@ -0,0 +1,5 @@
> +#!/bin/bash
> +#
> +# SPDX-License-Identifier: LGPL-2.1-only
> +#
> +./test/test-fcntl
> -- 
> 2.30.2
> 

Ping.

Mike.

Re: [OE-core] [pseudo][PATCH v2] fcntl: Add support for fcntl F_GETPIPE_SZ and F_SETPIPE_SZ

[Mike Crowe]

On Monday 06 September 2021 at 17:37:14 +0100, Mike Crowe via lists.openembedded.org wrote:
> On Friday 13 August 2021 at 12:05:09 +0100, Mike Crowe via lists.openembedded.org wrote:
> > When running the test suite on my Debian 11 box I see many occurrences
> > of:
> > 
> >  unknown fcntl argument 1032, assuming long argument.
> > 
> > (for example from test-execl.sh.)
> > 
> > It appears that this is F_GETPIPE_SZ and it takes no arguments. Let's
> > add it and the corresponding F_SETPIPE_SZ too to avoid the warning
> > messages.
> > 
> > F_SETPIPE_SZ accepts an int argument, which strictly speaking isn't the
> > same as the long that the wrapper expects. However, this is also true
> > for F_DUPFD which seems to be working correctly on all the targets that
> > people care about.
> > 
> > We need to define the command constants if the system headers don't
> > provide them to ensure that a binary built on an old system works
> > without the new commands works correctly only a newer one that tries to
> > use them. If the system values differ from the expected ones then such a
> > binary would also be incompatible, so fail the build in that case too.
> > 
> > Signed-off-by: Mike Crowe <mac@mcrowe.com>
> > ---
> >  Makefile.in              |  1 +
> >  ports/linux/guts/fcntl.c | 21 +++++++++++++++
> >  test/test-fcntl.c        | 58 ++++++++++++++++++++++++++++++++++++++++
> >  test/test-fcntl.sh       |  5 ++++
> >  4 files changed, 85 insertions(+)
> >  create mode 100644 test/test-fcntl.c
> >  create mode 100755 test/test-fcntl.sh

Hi Richard & Seebs,

It looks like only part of this change landed as
328452d74917ce9314c8c4afe2bd277473a4c076:

|  Makefile.in              |  1 +
|  ports/linux/guts/fcntl.c | 21 +++++++++++++++++++++

This means that the tests no longer pass:

cc: error: test/test-fcntl.c: No such file or directory

It looks like something similar happened with test-statx leading to
https://lists.openembedded.org/g/openembedded-core/message/154797 so I
wonder whether there's some sort of tooling fault somewhere?

Thanks.

Mike.

Re: [OE-core] [pseudo][PATCH v2] fcntl: Add support for fcntl F_GETPIPE_SZ and F_SETPIPE_SZ

[Richard Purdie]

On Fri, 2021-09-10 at 17:36 +0100, Mike Crowe wrote:
> On Monday 06 September 2021 at 17:37:14 +0100, Mike Crowe via lists.openembedded.org wrote:
> > On Friday 13 August 2021 at 12:05:09 +0100, Mike Crowe via lists.openembedded.org wrote:
> > > When running the test suite on my Debian 11 box I see many occurrences
> > > of:
> > > 
> > >  unknown fcntl argument 1032, assuming long argument.
> > > 
> > > (for example from test-execl.sh.)
> > > 
> > > It appears that this is F_GETPIPE_SZ and it takes no arguments. Let's
> > > add it and the corresponding F_SETPIPE_SZ too to avoid the warning
> > > messages.
> > > 
> > > F_SETPIPE_SZ accepts an int argument, which strictly speaking isn't the
> > > same as the long that the wrapper expects. However, this is also true
> > > for F_DUPFD which seems to be working correctly on all the targets that
> > > people care about.
> > > 
> > > We need to define the command constants if the system headers don't
> > > provide them to ensure that a binary built on an old system works
> > > without the new commands works correctly only a newer one that tries to
> > > use them. If the system values differ from the expected ones then such a
> > > binary would also be incompatible, so fail the build in that case too.
> > > 
> > > Signed-off-by: Mike Crowe <mac@mcrowe.com>
> > > ---
> > >  Makefile.in              |  1 +
> > >  ports/linux/guts/fcntl.c | 21 +++++++++++++++
> > >  test/test-fcntl.c        | 58 ++++++++++++++++++++++++++++++++++++++++
> > >  test/test-fcntl.sh       |  5 ++++
> > >  4 files changed, 85 insertions(+)
> > >  create mode 100644 test/test-fcntl.c
> > >  create mode 100755 test/test-fcntl.sh
> 
> Hi Richard & Seebs,
> 
> It looks like only part of this change landed as
> 328452d74917ce9314c8c4afe2bd277473a4c076:
> 
> >  Makefile.in              |  1 +
> >  ports/linux/guts/fcntl.c | 21 +++++++++++++++++++++
> 
> This means that the tests no longer pass:
> 
> cc: error: test/test-fcntl.c: No such file or directory
> 
> It looks like something similar happened with test-statx leading to
> https://lists.openembedded.org/g/openembedded-core/message/154797 so I
> wonder whether there's some sort of tooling fault somewhere?

Thanks, I've added those.

I really don't know what happened. This all gets tricky as I have to fix up the
email list mangling of your sender addresses, get these patches applied in a
pseudo recipe as patches, run test builds, then get the patches over to the
pseudo repo, push those then update the recipe.

I guess the issue is we don't run the pseudo tests anywhere so we don't spot
when they're missing. For most other issues I would see failures and fix things.
Not sure if/where we'd want to run them but it is something to think about. We
don't really have a good place for native tool testing.

Cheers,

Richard