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

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 02, 2022 at 04:59:14PM +0100, Jacopo Mondi wrote:
> On Fri, Dec 02, 2022 at 05:34:15PM +0200, Laurent Pinchart wrote:
> > 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 ?

I'd skip any C/C++ tools from utils completely on Android to be honest,
I don't think any of them will be run on an Android device.

> > > 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