[libcamera-devel] [PATCH] utils: ipu3: Include libgen.h for basename()

[Jacopo Mondi]

Hi Laurent

On Fri, Dec 02, 2022 at 05:34:15PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Dec 02, 2022 at 03:15:21PM +0100, Jacopo Mondi wrote:
> > On Fri, Dec 02, 2022 at 03:59:07PM +0200, Laurent Pinchart wrote:
> > > On Fri, Dec 02, 2022 at 02:48:27PM +0100, Jacopo Mondi wrote:
> > > > Not really...
> > > >
> > > > I get with clang
> > > >
> > > > ../utils/ipu3/ipu3-unpack.c:21:63: error: passing argument 1 of ‘__xpg_basename’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
> > > >    21 |         printf("Usage: %s input-file output-file\n", basename(argv0));
> > > >
> > > > Sorry, I should have tested before, but it seemed trivial.
> > > >
> > > > I now wonder what 'basename' we were calling, or better, to which
> > > > function prototype we were referring to, which didn't trigger such
> > > > error...
> > >
> > > From libgen.h:
> > >
> > > /* Return final component of PATH.
> > >
> > >    This is the weird XPG version of this function.  It sometimes will
> > >    modify its argument.  Therefore we normally use the GNU version (in
> > >    <string.h>) and only if this header is included make the XPG
> > >    version available under the real name.  */
> > > extern char *__xpg_basename (char *__path) __THROW;
> > > #define basename        __xpg_basename
> > >
> > > So we're using the string.h version. The basename manpage mentions
> >
> > Spot on, I read the header file after sending the email....
> >
> > >
> > > NOTES
> > >        There are two different versions of basename() - the POSIX version described above, and the GNU version, which one gets after
> > >
> > >                #define _GNU_SOURCE         /* See feature_test_macros(7) */
> > >                #include <string.h>
> > >
> > >        The GNU version never modifies its argument, and returns the empty string when path has a trailing slash, and in particular also when it is "/".  There is  no  GNU  version  of
> > >        dirname().
> > >
> > >        With glibc, one gets the POSIX version of basename() when <libgen.h> is included, and the GNU version otherwise.
> > >
> > > We have utils::basename() to support different libc versions, but we
> > > can't use it in utils/. Have you encountered this when compiling for
> > > Android ? Maybe we can disable compilation of those utilities instead ?
> > >
> >
> > Yes, I am trying compile with the NDK.
> >
> > I think we can safely remove those scripts from the build (I wonder
> > how this would be decided),
>
> Maybe a meson option ? Or doing so automatically based on a test at
> configuration time to see if there's a compliant basename() ? Or even a
> check on the platform type, if that's available.
>

We could indeed have a build option to compile the several platform
specific utilities. I can list utils/ipu3 and utils/rkisp1 while
utils/raspberry which contains CTT should probably be built
unconditionally ?

> > but I anticipate already that we'll have a
> > few hickups as in example:
> >
> > ../src/libcamera/media_device.cpp:167:2: error: use of undeclared identifier 'lockf'
> >         lockf(fd_.get(), F_ULOCK, 0);
> >
> > As Android has instead the BSD-conformant flock(). So expect more
> > wrappers in utils::  (hopefully not many)
>
> We could skip the whole locking on Android, as there will never be
> multiple processes trying to use the same camera. However, the lockf()
> issue seems weird, it seems to be available in bionic.
>

Only after API version 24 as far as I can tell, at least in NDK

#if __ANDROID_API__ >= 24
int lockf(int __fd, int __cmd, off_t __length) __RENAME_IF_FILE_OFFSET64(lockf64) __INTRODUCED_IN(24);

/**
 * Like lockf() but allows using a 64-bit length
 * even from a 32-bit process without `__FILE_OFFSET_BITS=64`.
 */
int lockf64(int __fd, int __cmd, off64_t __length) __INTRODUCED_IN(24);
#endif /* __ANDROID_API__ >= 24 */

I'll shortly send patches...

> > > > On Fri, Dec 02, 2022 at 03:33:56PM +0200, Laurent Pinchart wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Thank you for the patch.
> > > > >
> > > > > On Fri, Dec 02, 2022 at 02:27:06PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > > > > > The "basename" function from the standard C library is defined in the
> > > > > > libgen.h header.
> > > > > >
> > > > > > Include it to avoid errors triggered by -Wimplicit-function-declaration
> > > > > >
> > > > > > ../utils/ipu3/ipu3-pack.c:21:47: error: implicit declaration of function
> > > > > > 'basename' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > >
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > >
> > > > > > ---
> > > > > >  utils/ipu3/ipu3-pack.c   | 1 +
> > > > > >  utils/ipu3/ipu3-unpack.c | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/utils/ipu3/ipu3-pack.c b/utils/ipu3/ipu3-pack.c
> > > > > > index decbfc6c397a..27af44068956 100644
> > > > > > --- a/utils/ipu3/ipu3-pack.c
> > > > > > +++ b/utils/ipu3/ipu3-pack.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > >
> > > > > >  #include <errno.h>
> > > > > >  #include <fcntl.h>
> > > > > > +#include <libgen.h>
> > > > > >  #include <stdint.h>
> > > > > >  #include <stdio.h>
> > > > > >  #include <string.h>
> > > > > > diff --git a/utils/ipu3/ipu3-unpack.c b/utils/ipu3/ipu3-unpack.c
> > > > > > index 9d2c1200d932..aa67e0b0ecf5 100644
> > > > > > --- a/utils/ipu3/ipu3-unpack.c
> > > > > > +++ b/utils/ipu3/ipu3-unpack.c
> > > > > > @@ -8,6 +8,7 @@
> > > > > >
> > > > > >  #include <errno.h>
> > > > > >  #include <fcntl.h>
> > > > > > +#include <libgen.h>
> > > > > >  #include <stdint.h>
> > > > > >  #include <stdio.h>
> > > > > >  #include <string.h>
>
> --
> Regards,
>
> Laurent Pinchart