2019-10-07 04:13:19

by Masahiro Yamada

[permalink] [raw]
Subject: [RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

Currently, linux/videodev.h is excluded from the UAPI header test since
it is not self-contained. Building it for user-space would fail.

------------------------(build log1 begin)------------------------

CC usr/include/linux/videodev2.h.s
In file included from <command-line>:32:0:
./usr/include/linux/videodev2.h:2320:20: error: field ‘timestamp’ has incomplete type
struct timespec timestamp;
^~~~~~~~~

-------------------------(build log1 end)-------------------------

The in-kernel timespec definition exists in include/uapi/linux/time.h,
but just including <linux/time.h> causes a lot of redefinition errors.

------------------------(build log2 begin)------------------------

CC usr/include/linux/videodev2.h.s
In file included from ./usr/include/linux/videodev2.h:63:0,
from <command-line>:32:
./usr/include/linux/time.h:16:8: error: redefinition of ‘struct timeval’
struct timeval {
^~~~~~~
In file included from /usr/include/x86_64-linux-gnu/sys/time.h:25:0,
from ./usr/include/linux/videodev2.h:60,
from <command-line>:32:
/usr/include/x86_64-linux-gnu/bits/types/struct_timeval.h:8:8: note: originally defined here
struct timeval
^~~~~~~
In file included from ./usr/include/linux/videodev2.h:63:0,
from <command-line>:32:
./usr/include/linux/time.h:30:0: warning: "ITIMER_REAL" redefined
#define ITIMER_REAL 0

In file included from ./usr/include/linux/videodev2.h:60:0,
from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:92:0: note: this is the location of the previous definition
#define ITIMER_REAL ITIMER_REAL

In file included from ./usr/include/linux/videodev2.h:63:0,
from <command-line>:32:
./usr/include/linux/time.h:31:0: warning: "ITIMER_VIRTUAL" redefined
#define ITIMER_VIRTUAL 1

In file included from ./usr/include/linux/videodev2.h:60:0,
from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:95:0: note: this is the location of the previous definition
#define ITIMER_VIRTUAL ITIMER_VIRTUAL

In file included from ./usr/include/linux/videodev2.h:63:0,
from <command-line>:32:
./usr/include/linux/time.h:32:0: warning: "ITIMER_PROF" redefined
#define ITIMER_PROF 2

In file included from ./usr/include/linux/videodev2.h:60:0,
from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:99:0: note: this is the location of the previous definition
#define ITIMER_PROF ITIMER_PROF

In file included from ./usr/include/linux/videodev2.h:63:0,
from <command-line>:32:
./usr/include/linux/time.h:39:8: error: redefinition of ‘struct itimerval’
struct itimerval {
^~~~~~~~~
In file included from ./usr/include/linux/videodev2.h:60:0,
from <command-line>:32:
/usr/include/x86_64-linux-gnu/sys/time.h:104:8: note: originally defined here
struct itimerval
^~~~~~~~~

-------------------------(build log2 end)-------------------------

Replacing <sys/time.h> with <linux/time.h> solves it, and allow more
headers to join the UAPI header test.

Signed-off-by: Masahiro Yamada <[email protected]>
---

I am not 100% sure about the compatibility
between <sys/time.h> and <linux/time.h>, hence RFC.

But, if they were not compatible,
I guess it would have broken already.

I CCed Arnd Bergmann, who might have a better insight.

A related comment is here:
https://lkml.org/lkml/2019/6/4/1046


include/uapi/linux/videodev2.h | 4 +---
usr/include/Makefile | 7 -------
2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 530638dffd93..2571130aa1ca 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -57,11 +57,9 @@
#ifndef _UAPI__LINUX_VIDEODEV2_H
#define _UAPI__LINUX_VIDEODEV2_H

-#ifndef __KERNEL__
-#include <sys/time.h>
-#endif
#include <linux/compiler.h>
#include <linux/ioctl.h>
+#include <linux/time.h>
#include <linux/types.h>
#include <linux/v4l2-common.h>
#include <linux/v4l2-controls.h>
diff --git a/usr/include/Makefile b/usr/include/Makefile
index 57b20f7b6729..dafa6cb9b07e 100644
--- a/usr/include/Makefile
+++ b/usr/include/Makefile
@@ -23,7 +23,6 @@ header-test- += asm/shmbuf.h
header-test- += asm/signal.h
header-test- += asm/ucontext.h
header-test- += drm/vmwgfx_drm.h
-header-test- += linux/am437x-vpfe.h
header-test- += linux/android/binder.h
header-test- += linux/android/binderfs.h
header-test-$(CONFIG_CPU_BIG_ENDIAN) += linux/byteorder/big_endian.h
@@ -33,13 +32,10 @@ header-test- += linux/elfcore.h
header-test- += linux/errqueue.h
header-test- += linux/fsmap.h
header-test- += linux/hdlc/ioctl.h
-header-test- += linux/ivtv.h
header-test- += linux/kexec.h
-header-test- += linux/matroxfb.h
header-test- += linux/netfilter_ipv4/ipt_LOG.h
header-test- += linux/netfilter_ipv6/ip6t_LOG.h
header-test- += linux/nfc.h
-header-test- += linux/omap3isp.h
header-test- += linux/omapfb.h
header-test- += linux/patchkey.h
header-test- += linux/phonet.h
@@ -49,9 +45,6 @@ header-test- += linux/sctp.h
header-test- += linux/signal.h
header-test- += linux/sysctl.h
header-test- += linux/usb/audio.h
-header-test- += linux/v4l2-mediabus.h
-header-test- += linux/v4l2-subdev.h
-header-test- += linux/videodev2.h
header-test- += linux/vm_sockets.h
header-test- += sound/asequencer.h
header-test- += sound/asoc.h
--
2.17.1


2019-10-07 15:26:44

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

On Mon, Oct 7, 2019 at 6:10 AM Masahiro Yamada
<[email protected]> wrote:
>
> Currently, linux/videodev.h is excluded from the UAPI header test since
> it is not self-contained. Building it for user-space would fail.
>

>
> Replacing <sys/time.h> with <linux/time.h> solves it, and allow more
> headers to join the UAPI header test.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> I am not 100% sure about the compatibility
> between <sys/time.h> and <linux/time.h>, hence RFC.
>
> But, if they were not compatible,
> I guess it would have broken already.
>
> I CCed Arnd Bergmann, who might have a better insight.
>
> A related comment is here:
> https://lkml.org/lkml/2019/6/4/1046

I don't think this can work, there are multiple problems here:

* linux/time.h is still incompatible with sys/time.h, so any application
tries to include both sys/time.h and linux/videodev2.h now also
gets the compile-time error.

* The definition of 'struct timespec' in the kernel headers may in
fact be different from the one in the libc, and we do want to use
the one from the C library here, otherwise different parts of the
application may use incompatible struct layouts.

Fixing this correctly depends on one of the remaining y2038
patches that we still have to revisit. There are two aspects that
we should address:

1. The v4l subsystem needs to be changed to handle both the
old and the new 32-bit layout for timespec (and timeval). Both
Hans and I have created patches for this in the past, but they
were never completed and merged.

2. The definition of 'struct timespec' in the kernel headers needs
to be removed after every user of this struct is gone from
the kernel. In internal kernel code, the replacement is
timespec64 or ktime_t, and in user space interfaces, the
correct replacement is one of __kernel_timespec (the 64-bit
version), __kernel_old_timespec (the traditional layout) or
timespec (from the libc headers).

Arnd

2019-10-07 16:49:38

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [RFC PATCH] media: videodev2.h: include <linux/time.h> instead of <sys/time.h>

Hi Arnd,

On Tue, Oct 8, 2019 at 12:26 AM Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Oct 7, 2019 at 6:10 AM Masahiro Yamada
> <[email protected]> wrote:
> >
> > Currently, linux/videodev.h is excluded from the UAPI header test since
> > it is not self-contained. Building it for user-space would fail.
> >
>
> >
> > Replacing <sys/time.h> with <linux/time.h> solves it, and allow more
> > headers to join the UAPI header test.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > I am not 100% sure about the compatibility
> > between <sys/time.h> and <linux/time.h>, hence RFC.
> >
> > But, if they were not compatible,
> > I guess it would have broken already.
> >
> > I CCed Arnd Bergmann, who might have a better insight.
> >
> > A related comment is here:
> > https://lkml.org/lkml/2019/6/4/1046
>
> I don't think this can work, there are multiple problems here:
>
> * linux/time.h is still incompatible with sys/time.h, so any application
> tries to include both sys/time.h and linux/videodev2.h now also
> gets the compile-time error.

You are right.
I am so stupidly screwed up. Palm face...

Please ignore this patch,
and thank you for your expert comments!


>
> * The definition of 'struct timespec' in the kernel headers may in
> fact be different from the one in the libc, and we do want to use
> the one from the C library here, otherwise different parts of the
> application may use incompatible struct layouts.
>
> Fixing this correctly depends on one of the remaining y2038
> patches that we still have to revisit. There are two aspects that
> we should address:
>
> 1. The v4l subsystem needs to be changed to handle both the
> old and the new 32-bit layout for timespec (and timeval). Both
> Hans and I have created patches for this in the past, but they
> were never completed and merged.
>
> 2. The definition of 'struct timespec' in the kernel headers needs
> to be removed after every user of this struct is gone from
> the kernel. In internal kernel code, the replacement is
> timespec64 or ktime_t, and in user space interfaces, the
> correct replacement is one of __kernel_timespec (the 64-bit
> version), __kernel_old_timespec (the traditional layout) or
> timespec (from the libc headers).
>
> Arnd



--
Best Regards

Masahiro Yamada