v4l2: unset _TIME_BITS in addition to _FILE_OFFSET_BITS
diff mbox series

Message ID 20240320165840.35902-1-dylan.aissi@collabora.com
State Accepted
Commit 705601781b6ab4f3035719ddaf1e442f4a5e8737
Headers show
Series
  • v4l2: unset _TIME_BITS in addition to _FILE_OFFSET_BITS
Related show

Commit Message

Dylan Aïssi March 20, 2024, 4:58 p.m. UTC 
From: Steve Langasek <steve.langasek@canonical.com>

libcamera fails to build from source in Debian/Ubuntu on 32-bit
architectures under 64-bit time_t (to avoid the 'year 2038
problem'), because its v4l2 module legitimately un-sets
_FILE_OFFSET_BITS for building but this is not allowed without
also unsetting _TIME_BITS.

Having verified that nothing in this module is sensitive to 64-bit
time_t (none of the functions it intercepts handle time), we also
unset _TIME_BITS to allow this to build as before.

Signed-off-by: Steve Langasek <steve.langasek@canonical.com>
Reviewed-by: Dylan Aïssi <dylan.aissi@collabora.com>
---
 src/v4l2/meson.build | 1 +
 1 file changed, 1 insertion(+)

Comments

Kieran Bingham March 21, 2024, 9:21 a.m. UTC | #1 
Hi Steve, Dylan,

Quoting Dylan Aïssi (2024-03-20 16:58:40)
> From: Steve Langasek <steve.langasek@canonical.com>
> 
> libcamera fails to build from source in Debian/Ubuntu on 32-bit
> architectures under 64-bit time_t (to avoid the 'year 2038
> problem'), because its v4l2 module legitimately un-sets
> _FILE_OFFSET_BITS for building but this is not allowed without
> also unsetting _TIME_BITS.
> 
> Having verified that nothing in this module is sensitive to 64-bit
> time_t (none of the functions it intercepts handle time), we also
> unset _TIME_BITS to allow this to build as before.

Should we be setting -D_TIME_BITS=32 or anything like that, in the same
way that immediately after unsetting _FILE_OFFSET_BITS we set that to
32?

--
Kieran


> Signed-off-by: Steve Langasek <steve.langasek@canonical.com>
> Reviewed-by: Dylan Aïssi <dylan.aissi@collabora.com>
> ---
>  src/v4l2/meson.build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> index e88e0b33..12d1e2a4 100644
> --- a/src/v4l2/meson.build
> +++ b/src/v4l2/meson.build
> @@ -23,6 +23,7 @@ v4l2_compat_cpp_args = [
>      # file operations, disable transparent large file support.
>      '-U_FILE_OFFSET_BITS',
>      '-D_FILE_OFFSET_BITS=32',
> +    '-U_TIME_BITS',
>      '-D_LARGEFILE64_SOURCE',
>      '-fvisibility=hidden',
>  ]
> -- 
> 2.30.2
>
Steve Langasek March 22, 2024, 10:52 p.m. UTC | #2 
On Thu, Mar 21, 2024 at 09:21:19AM +0000, Kieran Bingham wrote:
> Hi Steve, Dylan,

> Quoting Dylan Aïssi (2024-03-20 16:58:40)
> > From: Steve Langasek <steve.langasek@canonical.com>

> > libcamera fails to build from source in Debian/Ubuntu on 32-bit
> > architectures under 64-bit time_t (to avoid the 'year 2038
> > problem'), because its v4l2 module legitimately un-sets
> > _FILE_OFFSET_BITS for building but this is not allowed without
> > also unsetting _TIME_BITS.

> > Having verified that nothing in this module is sensitive to 64-bit
> > time_t (none of the functions it intercepts handle time), we also
> > unset _TIME_BITS to allow this to build as before.

> Should we be setting -D_TIME_BITS=32 or anything like that, in the same
> way that immediately after unsetting _FILE_OFFSET_BITS we set that to
> 32?

No, there's no need to set it to another value.  The default behavior is for
it to be unset.  If some libc implementation other than glibc wants to set
it to a value other than 64, that's the business of that implementation.

I don't know why you're setting -D_FILE_OFFSET_BITS=32 either fwiw and think
this is probably wrong, but I was going for a minimal fix here for the
TIME/FILE bits incompatibility.

> > Signed-off-by: Steve Langasek <steve.langasek@canonical.com>
> > Reviewed-by: Dylan Aïssi <dylan.aissi@collabora.com>
> > ---
> >  src/v4l2/meson.build | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> > index e88e0b33..12d1e2a4 100644
> > --- a/src/v4l2/meson.build
> > +++ b/src/v4l2/meson.build
> > @@ -23,6 +23,7 @@ v4l2_compat_cpp_args = [
> >      # file operations, disable transparent large file support.
> >      '-U_FILE_OFFSET_BITS',
> >      '-D_FILE_OFFSET_BITS=32',
> > +    '-U_TIME_BITS',
> >      '-D_LARGEFILE64_SOURCE',
> >      '-fvisibility=hidden',
> >  ]
> > -- 
> > 2.30.2
> >
> 

Cheers,
Kieran Bingham March 26, 2024, 1:52 p.m. UTC | #3 
Quoting Steve Langasek (2024-03-22 22:52:54)
> On Thu, Mar 21, 2024 at 09:21:19AM +0000, Kieran Bingham wrote:
> > Hi Steve, Dylan,
> 
> > Quoting Dylan A�ssi (2024-03-20 16:58:40)
> > > From: Steve Langasek <steve.langasek@canonical.com>
> 
> > > libcamera fails to build from source in Debian/Ubuntu on 32-bit
> > > architectures under 64-bit time_t (to avoid the 'year 2038
> > > problem'), because its v4l2 module legitimately un-sets
> > > _FILE_OFFSET_BITS for building but this is not allowed without
> > > also unsetting _TIME_BITS.
> 
> > > Having verified that nothing in this module is sensitive to 64-bit
> > > time_t (none of the functions it intercepts handle time), we also
> > > unset _TIME_BITS to allow this to build as before.
> 
> > Should we be setting -D_TIME_BITS=32 or anything like that, in the same
> > way that immediately after unsetting _FILE_OFFSET_BITS we set that to
> > 32?
> 
> No, there's no need to set it to another value.  The default behavior is for
> it to be unset.  If some libc implementation other than glibc wants to set
> it to a value other than 64, that's the business of that implementation.
> 
> I don't know why you're setting -D_FILE_OFFSET_BITS=32 either fwiw and think
> this is probably wrong, but I was going for a minimal fix here for the
> TIME/FILE bits incompatibility.

I would suspect it's because we're implementing a kernel interface in
userspace, and we only implement a single version (32 bit). But that's
speculation - and probably Paul would know more.

 Cc: Paul

 
> > > Signed-off-by: Steve Langasek <steve.langasek@canonical.com>
> > > Reviewed-by: Dylan A�ssi <dylan.aissi@collabora.com>

Well, if you're sure the default is sufficient, I don't object to adding
this flag so:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> > > ---
> > >  src/v4l2/meson.build | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> > > index e88e0b33..12d1e2a4 100644
> > > --- a/src/v4l2/meson.build
> > > +++ b/src/v4l2/meson.build
> > > @@ -23,6 +23,7 @@ v4l2_compat_cpp_args = [
> > >      # file operations, disable transparent large file support.
> > >      '-U_FILE_OFFSET_BITS',
> > >      '-D_FILE_OFFSET_BITS=32',
> > > +    '-U_TIME_BITS',
> > >      '-D_LARGEFILE64_SOURCE',
> > >      '-fvisibility=hidden',
> > >  ]
> > > -- 
> > > 2.30.2
> > >
> > 
> 
> Cheers,
> -- 
> Steve Langasek                   Give me a lever long enough and a Free OS
> Debian Developer                   to set it on, and I can move the world.
> Ubuntu Developer                                   https://www.debian.org/
> slangasek@ubuntu.com                                     vorlon@debian.org
Laurent Pinchart March 26, 2024, 10:21 p.m. UTC | #4 
On Tue, Mar 26, 2024 at 01:52:46PM +0000, Kieran Bingham wrote:
> Quoting Steve Langasek (2024-03-22 22:52:54)
> > On Thu, Mar 21, 2024 at 09:21:19AM +0000, Kieran Bingham wrote:
> > > Quoting Dylan A�ssi (2024-03-20 16:58:40)
> > > > From: Steve Langasek <steve.langasek@canonical.com>
> > > >
> > > > libcamera fails to build from source in Debian/Ubuntu on 32-bit
> > > > architectures under 64-bit time_t (to avoid the 'year 2038
> > > > problem'), because its v4l2 module legitimately un-sets
> > > > _FILE_OFFSET_BITS for building but this is not allowed without
> > > > also unsetting _TIME_BITS.
> > > >
> > > > Having verified that nothing in this module is sensitive to 64-bit
> > > > time_t (none of the functions it intercepts handle time), we also
> > > > unset _TIME_BITS to allow this to build as before.
> > >
> > > Should we be setting -D_TIME_BITS=32 or anything like that, in the same
> > > way that immediately after unsetting _FILE_OFFSET_BITS we set that to
> > > 32?
> > 
> > No, there's no need to set it to another value.  The default behavior is for
> > it to be unset.  If some libc implementation other than glibc wants to set
> > it to a value other than 64, that's the business of that implementation.
> > 
> > I don't know why you're setting -D_FILE_OFFSET_BITS=32 either fwiw and think
> > this is probably wrong, but I was going for a minimal fix here for the
> > TIME/FILE bits incompatibility.
> 
> I would suspect it's because we're implementing a kernel interface in
> userspace, and we only implement a single version (32 bit).

We're intercepting both the 32-bit and 64-bit calls.

The glibc documentation
(https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html)
states


Macro: _FILE_OFFSET_BITS

    This macro determines which file system interface shall be used, one
    replacing the other. Whereas _LARGEFILE64_SOURCE makes the 64 bit
    interface available as an additional interface, _FILE_OFFSET_BITS
    allows the 64 bit interface to replace the old interface.

    If _FILE_OFFSET_BITS is defined to the value 32, the 32 bit
    interface is used and types like off_t have a size of 32 bits on 32
    bit systems.

    If the macro is defined to the value 64, the large file interface
    replaces the old interface. I.e., the functions are not made
    available under different names (as they are with
    _LARGEFILE64_SOURCE). Instead the old function names now reference
    the new functions, e.g., a call to fseeko now indeed calls fseeko64.

    If the macro is not defined it currently defaults to 32, but this
    default is planned to change due to a need to update time_t for
    Y2038 safety, and applications should not rely on the default.

    This macro should only be selected if the system provides mechanisms
    for handling large files. On 64 bit systems this macro has no effect
    since the *64 functions are identical to the normal functions.

    This macro was introduced as part of the Large File Support
    extension (LFS).


I think setting it to 32 explicitly is right in this case, as we do not
want glibc to redirect the 32 bit function calls to the 64 bit API. See
for instance how open() is handled:

#ifndef __USE_FILE_OFFSET64
extern int open (const char *__file, int __oflag, ...) __nonnull ((1));
#else
# ifdef __REDIRECT
extern int __REDIRECT (open, (const char *__file, int __oflag, ...), open64)
     __nonnull ((1));
# else
#  define open open64
# endif
#endif

where __USE_FILE_OFFSET64 is an internal macro that is defined i
_FILE_OFFSET_BITS==64. In that case, the libcamera function that
intercepts open() will be renamed to open64() by

#  define open open64

(_REDIRECT does something similar) and that would clash with the
redirection of open64().

Today we could leave _FILE_OFFSET_BITS undefined, but when the default
behaviour changes tomorrow, we'll see breakages.

> But that's
> speculation - and probably Paul would know more.
> 
>  Cc: Paul
>  
> > > > Signed-off-by: Steve Langasek <steve.langasek@canonical.com>
> > > > Reviewed-by: Dylan A�ssi <dylan.aissi@collabora.com>
> 
> Well, if you're sure the default is sufficient, I don't object to adding
> this flag so:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > > > ---
> > > >  src/v4l2/meson.build | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
> > > > index e88e0b33..12d1e2a4 100644
> > > > --- a/src/v4l2/meson.build
> > > > +++ b/src/v4l2/meson.build
> > > > @@ -23,6 +23,7 @@ v4l2_compat_cpp_args = [
> > > >      # file operations, disable transparent large file support.
> > > >      '-U_FILE_OFFSET_BITS',
> > > >      '-D_FILE_OFFSET_BITS=32',
> > > > +    '-U_TIME_BITS',
> > > >      '-D_LARGEFILE64_SOURCE',

Let's keep these alphabetically sorted, moving _TIME_BITS after
_LARGEFILE64_SOURCE. I'll change this when applying the patch, no need
to resubmit.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > >      '-fvisibility=hidden',
> > > >  ]

Patch
diff mbox series 

diff --git a/src/v4l2/meson.build b/src/v4l2/meson.build
index e88e0b33..12d1e2a4 100644
--- a/src/v4l2/meson.build
+++ b/src/v4l2/meson.build
@@ -23,6 +23,7 @@  v4l2_compat_cpp_args = [
     # file operations, disable transparent large file support.
     '-U_FILE_OFFSET_BITS',
     '-D_FILE_OFFSET_BITS=32',
+    '-U_TIME_BITS',
     '-D_LARGEFILE64_SOURCE',
     '-fvisibility=hidden',
 ]