[PATCH] v4l2: unset _TIME_BITS in addition to _FILE_OFFSET_BITS

[Laurent Pinchart]

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 at 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 at canonical.com>
> > > > Reviewed-by: Dylan A�ssi <dylan.aissi at 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 at 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 at ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart