All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
@ 2020-01-09  7:15 Li Wang
  2020-01-09  7:45 ` Petr Vorel
  2020-01-09  9:52 ` Cyril Hrubis
  0 siblings, 2 replies; 12+ messages in thread
From: Li Wang @ 2020-01-09  7:15 UTC (permalink / raw)
  To: ltp

To follow up commit 447c223dba538efc7be23edc.

Signed-off-by: Li Wang <liwang@redhat.com>
Tested-by: Li Wang <liwang@redhat.com>
Cc: Petr Vorel <pvorel@suse.cz>
Cc: Cyril Hrubis <chrubis@suse.cz>
---
 include/tst_device.h                                | 6 ++++--
 testcases/kernel/syscalls/fadvise/posix_fadvise01.c | 1 +
 testcases/kernel/syscalls/fadvise/posix_fadvise02.c | 1 +
 testcases/kernel/syscalls/fadvise/posix_fadvise03.c | 1 +
 testcases/kernel/syscalls/fadvise/posix_fadvise04.c | 1 +
 testcases/kernel/syscalls/fstat/fstat03.c           | 1 +
 testcases/kernel/syscalls/pwrite/pwrite02.c         | 1 +
 7 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/tst_device.h b/include/tst_device.h
index f277afd77..b4067be52 100644
--- a/include/tst_device.h
+++ b/include/tst_device.h
@@ -18,7 +18,9 @@
 #ifndef TST_DEVICE_H__
 #define TST_DEVICE_H__
 
+#define _GNU_SOURCE
 #include <unistd.h>
+#include <sys/syscall.h>
 
 struct tst_device {
 	const char *dev;
@@ -75,9 +77,9 @@ int tst_detach_device(const char *dev_path);
  * simply before the tst_dev_bytes_written invocation. For easy to use,
  * we create this inline function tst_dev_sync.
  */
-static inline void tst_dev_sync(int fd)
+static inline int tst_dev_sync(int fd)
 {
-	syncfs(fd);
+	return syscall(__NR_syncfs, fd);
 }
 
 /*
diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
index 2af040840..f5d7ca8ac 100644
--- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
+++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
@@ -20,6 +20,7 @@
  *	None
  */
 
+#define _GNU_SOURCE
 #define _XOPEN_SOURCE 600
 #include <fcntl.h>
 
diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise02.c b/testcases/kernel/syscalls/fadvise/posix_fadvise02.c
index d533a7953..899f58af8 100644
--- a/testcases/kernel/syscalls/fadvise/posix_fadvise02.c
+++ b/testcases/kernel/syscalls/fadvise/posix_fadvise02.c
@@ -20,6 +20,7 @@
  *	None
  */
 
+#define _GNU_SOURCE
 #define _XOPEN_SOURCE 600
 #include <fcntl.h>
 #include <unistd.h>
diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise03.c b/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
index 5bada5f3d..efd3ab378 100644
--- a/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
+++ b/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
@@ -20,6 +20,7 @@
  *	None
  */
 
+#define _GNU_SOURCE
 #define _XOPEN_SOURCE 600
 #include <fcntl.h>
 #include <unistd.h>
diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise04.c b/testcases/kernel/syscalls/fadvise/posix_fadvise04.c
index d8d8fb601..58162c6fb 100644
--- a/testcases/kernel/syscalls/fadvise/posix_fadvise04.c
+++ b/testcases/kernel/syscalls/fadvise/posix_fadvise04.c
@@ -20,6 +20,7 @@
  *	None
  */
 
+#define _GNU_SOURCE
 #define _XOPEN_SOURCE 600
 #include <fcntl.h>
 #include <unistd.h>
diff --git a/testcases/kernel/syscalls/fstat/fstat03.c b/testcases/kernel/syscalls/fstat/fstat03.c
index 68fae43df..51637cdd7 100644
--- a/testcases/kernel/syscalls/fstat/fstat03.c
+++ b/testcases/kernel/syscalls/fstat/fstat03.c
@@ -13,6 +13,7 @@
  *    -> EFAULT (or receive signal SIGSEGV)
  */
 
+#define _GNU_SOURCE
 #include <errno.h>
 #include <stdlib.h>
 #include <unistd.h>
diff --git a/testcases/kernel/syscalls/pwrite/pwrite02.c b/testcases/kernel/syscalls/pwrite/pwrite02.c
index 056d44da2..4582d4e94 100644
--- a/testcases/kernel/syscalls/pwrite/pwrite02.c
+++ b/testcases/kernel/syscalls/pwrite/pwrite02.c
@@ -18,6 +18,7 @@
  *      accessible address space, returns EFAULT.
  */
 
+#define _GNU_SOURCE
 #define _XOPEN_SOURCE 500
 
 #include <errno.h>
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09  7:15 [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h Li Wang
@ 2020-01-09  7:45 ` Petr Vorel
  2020-01-09  8:02   ` Li Wang
  2020-01-09  9:52 ` Cyril Hrubis
  1 sibling, 1 reply; 12+ messages in thread
From: Petr Vorel @ 2020-01-09  7:45 UTC (permalink / raw)
  To: ltp

Hi Li,

> To follow up commit 447c223dba538efc7be23edc.

> Signed-off-by: Li Wang <liwang@redhat.com>
> Tested-by: Li Wang <liwang@redhat.com>
> Cc: Petr Vorel <pvorel@suse.cz>
BTW I wonder if my sieve filter is bad, because I didn't get this mail or git
send-email didn't sent it.

> Cc: Cyril Hrubis <chrubis@suse.cz>

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Nice, thanks for a fix. + Note about _XOPEN_SOURCE below.

Tested-by: Petr Vorel <pvorel@suse.cz>
https://travis-ci.org/pevik/ltp/builds/634613112
NOTE: it works, failures to Debian testing are caused by bug in libtirpc 1.2.5,
fixed in a86b4ff Add authdes_seccreate() stub [1].

...
> diff --git a/include/tst_device.h b/include/tst_device.h
> index f277afd77..b4067be52 100644
> --- a/include/tst_device.h
> +++ b/include/tst_device.h
> @@ -18,7 +18,9 @@
>  #ifndef TST_DEVICE_H__
>  #define TST_DEVICE_H__

> +#define _GNU_SOURCE
>  #include <unistd.h>
> +#include <sys/syscall.h>

>  struct tst_device {
>  	const char *dev;
> @@ -75,9 +77,9 @@ int tst_detach_device(const char *dev_path);
>   * simply before the tst_dev_bytes_written invocation. For easy to use,
>   * we create this inline function tst_dev_sync.
>   */
> -static inline void tst_dev_sync(int fd)
> +static inline int tst_dev_sync(int fd)
>  {
> -	syncfs(fd);
> +	return syscall(__NR_syncfs, fd);
+1 for returning result.
>  }

>  /*
> diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> index 2af040840..f5d7ca8ac 100644
> --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> @@ -20,6 +20,7 @@
>   *	None
>   */

> +#define _GNU_SOURCE
>  #define _XOPEN_SOURCE 600
If we define _GNU_SOURCE we probably don't need _XOPEN_SOURCE 600, do we?

>  #include <fcntl.h>

> diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise02.c b/testcases/kernel/syscalls/fadvise/posix_fadvise02.c
> index d533a7953..899f58af8 100644
> --- a/testcases/kernel/syscalls/fadvise/posix_fadvise02.c
> +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise02.c
> @@ -20,6 +20,7 @@
>   *	None
>   */

> +#define _GNU_SOURCE
>  #define _XOPEN_SOURCE 600
Same here.

>  #include <fcntl.h>
>  #include <unistd.h>
> diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise03.c b/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
> index 5bada5f3d..efd3ab378 100644
> --- a/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
> +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
> @@ -20,6 +20,7 @@
>   *	None
>   */

> +#define _GNU_SOURCE
>  #define _XOPEN_SOURCE 600
Same here.

...
> diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise04.c b/testcases/kernel/syscalls/fadvise/posix_fadvise04.c
> index d8d8fb601..58162c6fb 100644
> --- a/testcases/kernel/syscalls/fadvise/posix_fadvise04.c
> +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise04.c
> @@ -20,6 +20,7 @@
>   *	None
>   */

> +#define _GNU_SOURCE
>  #define _XOPEN_SOURCE 600
Same here.
>  #include <fcntl.h>
>  #include <unistd.h>

...
> diff --git a/testcases/kernel/syscalls/pwrite/pwrite02.c b/testcases/kernel/syscalls/pwrite/pwrite02.c
> index 056d44da2..4582d4e94 100644
> --- a/testcases/kernel/syscalls/pwrite/pwrite02.c
> +++ b/testcases/kernel/syscalls/pwrite/pwrite02.c
> @@ -18,6 +18,7 @@
>   *      accessible address space, returns EFAULT.
>   */

> +#define _GNU_SOURCE
>  #define _XOPEN_SOURCE 500
Same here.

Kind regards,
Petr

[1] http://git.linux-nfs.org/?p=steved/libtirpc.git;a=commit;h=a86b4ff0c4b4e53df436f83c21a5fbf01568a301

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09  7:45 ` Petr Vorel
@ 2020-01-09  8:02   ` Li Wang
  2020-01-09  8:21     ` Petr Vorel
  0 siblings, 1 reply; 12+ messages in thread
From: Li Wang @ 2020-01-09  8:02 UTC (permalink / raw)
  To: ltp

Hi Petr,

On Thu, Jan 9, 2020 at 3:45 PM Petr Vorel <pvorel@suse.cz> wrote:

> ...
> > Cc: Petr Vorel <pvorel@suse.cz>
> BTW I wonder if my sieve filter is bad, because I didn't get this mail or
> git
> send-email didn't sent it.
>

I guess that might because of your email filter configuration, the
Cc'ed name will be kicked out if he/she has subscribed to the mailing list,
especially for my Gmail-client always do that.

This is git-send-email log FYI:
----------------------------
$ git send-email 0001-tst_device-use-raw-syscall-in-the-tst_device.h.patch
--to ltp@lists.linux.it
0001-tst_device-use-raw-syscall-in-the-tst_device.h.patch
(body) Adding cc: Petr Vorel <pvorel@suse.cz> from line 'Cc: Petr Vorel <
pvorel@suse.cz>'
(body) Adding cc: Cyril Hrubis <chrubis@suse.cz> from line 'Cc: Cyril
Hrubis <chrubis@suse.cz>'
OK. Log says:
Server: smtp.corp.redhat.com
MAIL FROM:<liwang@redhat.com>
RCPT TO:<ltp@lists.linux.it>
RCPT TO:<pvorel@suse.cz>
RCPT TO:<chrubis@suse.cz>
From: Li Wang <liwang@redhat.com>
To: ltp@lists.linux.it
Cc: Petr Vorel <pvorel@suse.cz>,
Cyril Hrubis <chrubis@suse.cz>
Subject: [PATCH] tst_device: use raw syscall in the tst_device.h
Date: Thu,  9 Jan 2020 15:15:10 +0800
Message-Id: <20200109071510.11223-1-liwang@redhat.com>
X-Mailer: git-send-email 2.20.1
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Result: 250


> > Cc: Cyril Hrubis <chrubis@suse.cz>
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Nice, thanks for a fix. + Note about _XOPEN_SOURCE below.
>

Thanks for the quick reply.


>
> Tested-by: Petr Vorel <pvorel@suse.cz>
> https://travis-ci.org/pevik/ltp/builds/634613112
> NOTE: it works, failures to Debian testing are caused by bug in libtirpc
> 1.2.5,
> fixed in a86b4ff Add authdes_seccreate() stub [1].
>
> ...

> +#define _GNU_SOURCE
> >  #define _XOPEN_SOURCE 600
> If we define _GNU_SOURCE we probably don't need _XOPEN_SOURCE 600, do we?
>

Ah, right. I will remove it in patch merging.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200109/d54007f0/attachment.htm>

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09  8:02   ` Li Wang
@ 2020-01-09  8:21     ` Petr Vorel
  0 siblings, 0 replies; 12+ messages in thread
From: Petr Vorel @ 2020-01-09  8:21 UTC (permalink / raw)
  To: ltp

Hi Li,

> > > Cc: Petr Vorel <pvorel@suse.cz>
> > BTW I wonder if my sieve filter is bad, because I didn't get this mail or
> > git
> > send-email didn't sent it.


> I guess that might because of your email filter configuration, the
> Cc'ed name will be kicked out if he/she has subscribed to the mailing list,
> especially for my Gmail-client always do that.

> This is git-send-email log FYI:
> ----------------------------
> $ git send-email 0001-tst_device-use-raw-syscall-in-the-tst_device.h.patch
> --to ltp@lists.linux.it
> 0001-tst_device-use-raw-syscall-in-the-tst_device.h.patch
> (body) Adding cc: Petr Vorel <pvorel@suse.cz> from line 'Cc: Petr Vorel <
> pvorel@suse.cz>'
> (body) Adding cc: Cyril Hrubis <chrubis@suse.cz> from line 'Cc: Cyril
> Hrubis <chrubis@suse.cz>'
> OK. Log says:
> Server: smtp.corp.redhat.com
> MAIL FROM:<liwang@redhat.com>
> RCPT TO:<ltp@lists.linux.it>
> RCPT TO:<pvorel@suse.cz>
> RCPT TO:<chrubis@suse.cz>
> From: Li Wang <liwang@redhat.com>
> To: ltp@lists.linux.it
> Cc: Petr Vorel <pvorel@suse.cz>,
> Cyril Hrubis <chrubis@suse.cz>
> Subject: [PATCH] tst_device: use raw syscall in the tst_device.h
> Date: Thu,  9 Jan 2020 15:15:10 +0800
> Message-Id: <20200109071510.11223-1-liwang@redhat.com>
> X-Mailer: git-send-email 2.20.1
> MIME-Version: 1.0
> Content-Transfer-Encoding: 8bit

> Result: 250

Thank you for info. Sorry, my fault (I had filter to List-Id: and *also* to To:
(normally I use just List-Id:, which is what I want).

> > +#define _GNU_SOURCE
> > >  #define _XOPEN_SOURCE 600
> > If we define _GNU_SOURCE we probably don't need _XOPEN_SOURCE 600, do we?


> Ah, right. I will remove it in patch merging.
Thanks!

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09  7:15 [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h Li Wang
  2020-01-09  7:45 ` Petr Vorel
@ 2020-01-09  9:52 ` Cyril Hrubis
  2020-01-09  9:59   ` Petr Vorel
  2020-01-09 12:41   ` Li Wang
  1 sibling, 2 replies; 12+ messages in thread
From: Cyril Hrubis @ 2020-01-09  9:52 UTC (permalink / raw)
  To: ltp

Hi!
> +#define _GNU_SOURCE
>  #include <unistd.h>
> +#include <sys/syscall.h>

Defining _GNU_SOURCE anywhere but at the top of the test source is
meaningless. It has to be defined before we include any libc headers
otherwise it's ignored.

>  struct tst_device {
>  	const char *dev;
> @@ -75,9 +77,9 @@ int tst_detach_device(const char *dev_path);
>   * simply before the tst_dev_bytes_written invocation. For easy to use,
>   * we create this inline function tst_dev_sync.
>   */
> -static inline void tst_dev_sync(int fd)
> +static inline int tst_dev_sync(int fd)
>  {
> -	syncfs(fd);
> +	return syscall(__NR_syncfs, fd);
>  }
>  
>  /*
> diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> index 2af040840..f5d7ca8ac 100644
> --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> @@ -20,6 +20,7 @@
>   *	None
>   */
>  
> +#define _GNU_SOURCE
>  #define _XOPEN_SOURCE 600
>  #include <fcntl.h>

Why do we need the _GNU_SOURCE here? We switched to a syscall() in the
header, hence we do not need the syncfs() prototype anymore.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09  9:52 ` Cyril Hrubis
@ 2020-01-09  9:59   ` Petr Vorel
  2020-01-09 12:41   ` Li Wang
  1 sibling, 0 replies; 12+ messages in thread
From: Petr Vorel @ 2020-01-09  9:59 UTC (permalink / raw)
  To: ltp

Hi Cyril, Li,

> Hi!
> > +#define _GNU_SOURCE
> >  #include <unistd.h>
> > +#include <sys/syscall.h>

> Defining _GNU_SOURCE anywhere but at the top of the test source is
> meaningless. It has to be defined before we include any libc headers
> otherwise it's ignored.

> >  struct tst_device {
> >  	const char *dev;
> > @@ -75,9 +77,9 @@ int tst_detach_device(const char *dev_path);
> >   * simply before the tst_dev_bytes_written invocation. For easy to use,
> >   * we create this inline function tst_dev_sync.
> >   */
> > -static inline void tst_dev_sync(int fd)
> > +static inline int tst_dev_sync(int fd)
> >  {
> > -	syncfs(fd);
> > +	return syscall(__NR_syncfs, fd);
> >  }

> > diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c

> > +#define _GNU_SOURCE
> >  #define _XOPEN_SOURCE 600
> >  #include <fcntl.h>

> Why do we need the _GNU_SOURCE here? We switched to a syscall() in the
> header, hence we do not need the syncfs() prototype anymore.

Correct, both. Sorry for a complete wrong review.

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09  9:52 ` Cyril Hrubis
  2020-01-09  9:59   ` Petr Vorel
@ 2020-01-09 12:41   ` Li Wang
  2020-01-09 12:49     ` Cyril Hrubis
  2020-01-09 12:56     ` Petr Vorel
  1 sibling, 2 replies; 12+ messages in thread
From: Li Wang @ 2020-01-09 12:41 UTC (permalink / raw)
  To: ltp

On Thu, Jan 9, 2020 at 5:52 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > +#define _GNU_SOURCE
> >  #include <unistd.h>
> > +#include <sys/syscall.h>
>
> Defining _GNU_SOURCE anywhere but at the top of the test source is
> meaningless. It has to be defined before we include any libc headers
> otherwise it's ignored.
>

I got the point. And yes, that means the definition should be removed from
the tst_device.h header file.


> >  /*
> > diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > index 2af040840..f5d7ca8ac 100644
> > --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > @@ -20,6 +20,7 @@
> >   *   None
> >   */
> >
> > +#define _GNU_SOURCE
> >  #define _XOPEN_SOURCE 600
> >  #include <fcntl.h>
>
> Why do we need the _GNU_SOURCE here? We switched to a syscall() in the
> header, hence we do not need the syncfs() prototype anymore.
>
>
But shouldn't we define the _GNU_SOURCE for syscall()?  Otherwise,
the _XOPEN_SOURCE 600 definitions will take effect and makes the compiler
print new errors[1].

Here I fee a little confused, or do we need to delete the _XOPEN_SOURCE
definition directly for the test posix_fadvise01.c?

[1]:
gcc -Werror=implicit-function-declaration -g -O2 -fno-strict-aliasing -pipe
-Wall -W -Wold-style-definition
 -I/root/ltp2/testcases/kernel/syscalls/fadvise
-I/root/ltp2/testcases/kernel/syscalls/fadvise/../utils
-I../../../../include -I../../../../include -I../../../../include/old/
-L../../../../lib  posix_fadvise01.c   -lltp -o posix_fadvise01
In file included from ../../../../include/tst_test.h:22,
                 from posix_fadvise01.c:31:
../../../../include/tst_device.h: In function ?tst_dev_sync?:
../../../../include/tst_device.h:82:9: error: implicit declaration of
function ?syscall?; did you mean ?strcoll??
[-Werror=implicit-function-declaration]
  return syscall(__NR_syncfs, fd);
         ^~~~~~~
         strcoll

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200109/c446e9d2/attachment.htm>

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09 12:41   ` Li Wang
@ 2020-01-09 12:49     ` Cyril Hrubis
  2020-01-09 12:56       ` Li Wang
  2020-01-09 12:56     ` Petr Vorel
  1 sibling, 1 reply; 12+ messages in thread
From: Cyril Hrubis @ 2020-01-09 12:49 UTC (permalink / raw)
  To: ltp

Hi!
> > > diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > index 2af040840..f5d7ca8ac 100644
> > > --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > @@ -20,6 +20,7 @@
> > >   *   None
> > >   */
> > >
> > > +#define _GNU_SOURCE
> > >  #define _XOPEN_SOURCE 600
> > >  #include <fcntl.h>
> >
> > Why do we need the _GNU_SOURCE here? We switched to a syscall() in the
> > header, hence we do not need the syncfs() prototype anymore.
> >
> >
> But shouldn't we define the _GNU_SOURCE for syscall()?  Otherwise,
> the _XOPEN_SOURCE 600 definitions will take effect and makes the compiler
> print new errors[1].
> 
> Here I fee a little confused, or do we need to delete the _XOPEN_SOURCE
> definition directly for the test posix_fadvise01.c?

Sigh, looks like the _XOPEN_SOURCE 600 disables syscall() definition
from being exposed, which is otherwise exposed by default.

Also it looks like things works fine for me when I remove the
_XOPEN_SOURCE 600.

So I guess that we should try to remove the _XOPEN_SOURCE from the
posix_fadvise() tests and try to compile the code on old enough
distribution. If that works we should do it that way.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09 12:41   ` Li Wang
  2020-01-09 12:49     ` Cyril Hrubis
@ 2020-01-09 12:56     ` Petr Vorel
  2020-01-09 13:10       ` Cyril Hrubis
  1 sibling, 1 reply; 12+ messages in thread
From: Petr Vorel @ 2020-01-09 12:56 UTC (permalink / raw)
  To: ltp

Hi,

> > > +#define _GNU_SOURCE
> > >  #include <unistd.h>
> > > +#include <sys/syscall.h>

> > Defining _GNU_SOURCE anywhere but at the top of the test source is
> > meaningless. It has to be defined before we include any libc headers
> > otherwise it's ignored.


> I got the point. And yes, that means the definition should be removed from
> the tst_device.h header file.


> > > diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
...
> > > +#define _GNU_SOURCE
> > >  #define _XOPEN_SOURCE 600
> > >  #include <fcntl.h>

> > Why do we need the _GNU_SOURCE here? We switched to a syscall() in the
> > header, hence we do not need the syncfs() prototype anymore.


> But shouldn't we define the _GNU_SOURCE for syscall()?  Otherwise,
> the _XOPEN_SOURCE 600 definitions will take effect and makes the compiler
> print new errors[1].
Correct, syscall() requires _GNU_SOURCE and <unistd.h>.
+ Not sure if <sys/syscall.h> should be used (as it's in your patch or
lapi/syscalls.h.

> Here I feel a little confused, or do we need to delete the _XOPEN_SOURCE
> definition directly for the test posix_fadvise01.c?

> [1]:
> gcc -Werror=implicit-function-declaration -g -O2 -fno-strict-aliasing -pipe
> -Wall -W -Wold-style-definition
>  -I/root/ltp2/testcases/kernel/syscalls/fadvise
> -I/root/ltp2/testcases/kernel/syscalls/fadvise/../utils
> -I../../../../include -I../../../../include -I../../../../include/old/
> -L../../../../lib  posix_fadvise01.c   -lltp -o posix_fadvise01
> In file included from ../../../../include/tst_test.h:22,
>                  from posix_fadvise01.c:31:
> ../../../../include/tst_device.h: In function ?tst_dev_sync?:
> ../../../../include/tst_device.h:82:9: error: implicit declaration of
> function ?syscall?; did you mean ?strcoll??
> [-Werror=implicit-function-declaration]
>   return syscall(__NR_syncfs, fd);
>          ^~~~~~~
>          strcoll

+ Our syscall numbers in include/lapi/syscalls/ are outdated (syncfs is not at
least in include/lapi/syscalls/sparc{64,}.in and
include/lapi/syscalls/x86_64.in).

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09 12:49     ` Cyril Hrubis
@ 2020-01-09 12:56       ` Li Wang
  0 siblings, 0 replies; 12+ messages in thread
From: Li Wang @ 2020-01-09 12:56 UTC (permalink / raw)
  To: ltp

On Thu, Jan 9, 2020 at 8:49 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > > diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > > index 2af040840..f5d7ca8ac 100644
> > > > --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > > +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > > > @@ -20,6 +20,7 @@
> > > >   *   None
> > > >   */
> > > >
> > > > +#define _GNU_SOURCE
> > > >  #define _XOPEN_SOURCE 600
> > > >  #include <fcntl.h>
> > >
> > > Why do we need the _GNU_SOURCE here? We switched to a syscall() in the
> > > header, hence we do not need the syncfs() prototype anymore.
> > >
> > >
> > But shouldn't we define the _GNU_SOURCE for syscall()?  Otherwise,
> > the _XOPEN_SOURCE 600 definitions will take effect and makes the compiler
> > print new errors[1].
> >
> > Here I fee a little confused, or do we need to delete the _XOPEN_SOURCE
> > definition directly for the test posix_fadvise01.c?
>
> Sigh, looks like the _XOPEN_SOURCE 600 disables syscall() definition
> from being exposed, which is otherwise exposed by default.
>
> Also it looks like things works fine for me when I remove the
> _XOPEN_SOURCE 600.
>
> So I guess that we should try to remove the _XOPEN_SOURCE from the
> posix_fadvise() tests and try to compile the code on old enough
> distribution. If that works we should do it that way.
>

Ok, I will have a try (remove the _XOPEN_SOURCE) to build it on more
distros. If works fine will send patch v2.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200109/9e1d1ba2/attachment-0001.htm>

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09 12:56     ` Petr Vorel
@ 2020-01-09 13:10       ` Cyril Hrubis
  2020-01-09 14:28         ` Petr Vorel
  0 siblings, 1 reply; 12+ messages in thread
From: Cyril Hrubis @ 2020-01-09 13:10 UTC (permalink / raw)
  To: ltp

Hi!
> > But shouldn't we define the _GNU_SOURCE for syscall()?  Otherwise,
> > the _XOPEN_SOURCE 600 definitions will take effect and makes the compiler
> > print new errors[1].
> Correct, syscall() requires _GNU_SOURCE and <unistd.h>.

Not really, it's guarded by _USE_MISC which is enabled by default and
disabled by _XOPEN_SOURCE.

Looks like the manual page is outdated at least.

> + Not sure if <sys/syscall.h> should be used (as it's in your patch or
> lapi/syscalls.h.

Well, given that syncfs is old enough (2.6.39) either should work.

> > Here I feel a little confused, or do we need to delete the _XOPEN_SOURCE
> > definition directly for the test posix_fadvise01.c?
> 
> > [1]:
> > gcc -Werror=implicit-function-declaration -g -O2 -fno-strict-aliasing -pipe
> > -Wall -W -Wold-style-definition
> >  -I/root/ltp2/testcases/kernel/syscalls/fadvise
> > -I/root/ltp2/testcases/kernel/syscalls/fadvise/../utils
> > -I../../../../include -I../../../../include -I../../../../include/old/
> > -L../../../../lib  posix_fadvise01.c   -lltp -o posix_fadvise01
> > In file included from ../../../../include/tst_test.h:22,
> >                  from posix_fadvise01.c:31:
> > ../../../../include/tst_device.h: In function ???tst_dev_sync???:
> > ../../../../include/tst_device.h:82:9: error: implicit declaration of
> > function ???syscall???; did you mean ???strcoll????
> > [-Werror=implicit-function-declaration]
> >   return syscall(__NR_syncfs, fd);
> >          ^~~~~~~
> >          strcoll
> 
> + Our syscall numbers in include/lapi/syscalls/ are outdated (syncfs is not at
> least in include/lapi/syscalls/sparc{64,}.in and
> include/lapi/syscalls/x86_64.in).

The lapi/syscall.h includes sys/syscall.h so it's probably not a
problem, since there are no distribution that are missing syncfs in the
system headers. Note that we are only adding fallback definitions there
and if present the system values take precedence.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h
  2020-01-09 13:10       ` Cyril Hrubis
@ 2020-01-09 14:28         ` Petr Vorel
  0 siblings, 0 replies; 12+ messages in thread
From: Petr Vorel @ 2020-01-09 14:28 UTC (permalink / raw)
  To: ltp

Hi,

> > > But shouldn't we define the _GNU_SOURCE for syscall()?  Otherwise,
> > > the _XOPEN_SOURCE 600 definitions will take effect and makes the compiler
> > > print new errors[1].
> > Correct, syscall() requires _GNU_SOURCE and <unistd.h>.

> Not really, it's guarded by _USE_MISC which is enabled by default and
> disabled by _XOPEN_SOURCE.
Yep, looking into features.h __USE_MISC is defined by _DEFAULT_SOURCE,
which is enabled if nothing or only _GNU_SOURCE is defined.

> Looks like the manual page is outdated at least.
Yes, that's what I checked this time (I usually look into sources).

> > + Not sure if <sys/syscall.h> should be used (as it's in your patch or
> > lapi/syscalls.h.

> Well, given that syncfs is old enough (2.6.39) either should work.

> > > Here I feel a little confused, or do we need to delete the _XOPEN_SOURCE
> > > definition directly for the test posix_fadvise01.c?

> > > [1]:
> > > gcc -Werror=implicit-function-declaration -g -O2 -fno-strict-aliasing -pipe
> > > -Wall -W -Wold-style-definition
> > >  -I/root/ltp2/testcases/kernel/syscalls/fadvise
> > > -I/root/ltp2/testcases/kernel/syscalls/fadvise/../utils
> > > -I../../../../include -I../../../../include -I../../../../include/old/
> > > -L../../../../lib  posix_fadvise01.c   -lltp -o posix_fadvise01
> > > In file included from ../../../../include/tst_test.h:22,
> > >                  from posix_fadvise01.c:31:
> > > ../../../../include/tst_device.h: In function ???tst_dev_sync???:
> > > ../../../../include/tst_device.h:82:9: error: implicit declaration of
> > > function ???syscall???; did you mean ???strcoll????
> > > [-Werror=implicit-function-declaration]
> > >   return syscall(__NR_syncfs, fd);
> > >          ^~~~~~~
> > >          strcoll

> > + Our syscall numbers in include/lapi/syscalls/ are outdated (syncfs is not at
> > least in include/lapi/syscalls/sparc{64,}.in and
> > include/lapi/syscalls/x86_64.in).

> The lapi/syscall.h includes sys/syscall.h so it's probably not a
> problem, since there are no distribution that are missing syncfs in the
> system headers. Note that we are only adding fallback definitions there
> and if present the system values take precedence.
Good. Sorry for wrong report (haven't notice <sys/syscall.h> inclusion).

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-01-09 14:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-09  7:15 [LTP] [PATCH] tst_device: use raw syscall in the tst_device.h Li Wang
2020-01-09  7:45 ` Petr Vorel
2020-01-09  8:02   ` Li Wang
2020-01-09  8:21     ` Petr Vorel
2020-01-09  9:52 ` Cyril Hrubis
2020-01-09  9:59   ` Petr Vorel
2020-01-09 12:41   ` Li Wang
2020-01-09 12:49     ` Cyril Hrubis
2020-01-09 12:56       ` Li Wang
2020-01-09 12:56     ` Petr Vorel
2020-01-09 13:10       ` Cyril Hrubis
2020-01-09 14:28         ` Petr Vorel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.