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

[Laurent Pinchart]

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.

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

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