diff mbox series

[PATCH/next,1/2] package/libcap: drop host-gperf dependency

Message ID 20200830100058.4180798-1-fontaine.fabrice@gmail.com
State New
Headers show
Series [PATCH/next,1/2] package/libcap: drop host-gperf dependency | expand

Commit Message

Fabrice Fontaine Aug. 30, 2020, 10 a.m. UTC
host-gperf dependency was added in commit
5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the
commit message but gperf can be disabled through BUILD_GPERF since
https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba

So use this variable and drop this unneeded dependency

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
 package/libcap/libcap.mk | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Thomas Petazzoni Sept. 3, 2020, 8:47 p.m. UTC | #1
Hello Fabrice,

On Sun, 30 Aug 2020 12:00:57 +0200
Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:

> host-gperf dependency was added in commit
> 5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the
> commit message but gperf can be disabled through BUILD_GPERF since
> https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba
> 
> So use this variable and drop this unneeded dependency
> 
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>

Thanks for looking into this. I agree that BUILD_GPERF=no will avoid
the host-gperf dependency, however I'm a bit confused how this works in
libcap and whether it has any drawback.

When gperf is detected, it will be used to generate some C code that
contains a hash function:

ifeq ($(BUILD_GPERF),yes)
USE_GPERF_OUTPUT = $(GPERF_OUTPUT)
INCLUDE_GPERF_OUTPUT = -DINCLUDE_GPERF_OUTPUT='"$(GPERF_OUTPUT)"'
endif

$(GPERF_OUTPUT): cap_names.list.h
        perl -e 'print "struct __cap_token_s { const char *name; int index; };\n%{\nconst struct __cap_token_s *__cap_lookup_name(const char *, size_t);\n%}\n%%\n"; while ($$l = <>) { $$l =~ s/[\{\"]//g; $$l =~ s/\}.*// ; print $$l; }' < $< | gperf --ignore-case --language=ANSI-C --readonly --null-strings --global-table --hash-function-name=__cap_hash_name --lookup-function-name="__cap_lookup_name" -c -t -m20 $(INDENT) > $@
        sed -e 's/unsigned int len/size_t len/' -i $@

cap_text.o: cap_text.c $(USE_GPERF_OUTPUT) $(INCLS)
        $(CC) $(CFLAGS) $(IPATH) $(INCLUDE_GPERF_OUTPUT) -c $< -o $@

Then in cap_text.c, this gets used like this:

#ifdef INCLUDE_GPERF_OUTPUT
/* we need to include it after #define _GNU_SOURCE is set */
#include INCLUDE_GPERF_OUTPUT
#endif

So the gperf-generated source file, if it exists is included. But I
don't see any conditional code that makes use of it when available.

Am I missing something? I wanted to understand the impact of not having
gperf. Perhaps not having a hash function makes libcap significantly
slower ?

Initially, I was expected libcap to have a pre-generated version of the
file, and having gperf available would only allow to re-generate the
file, but that doesn't seem to be what's happening.

Do you understand a bit better what is going on here ?

Thanks,

Thomas
Fabrice Fontaine Sept. 5, 2020, 10:19 a.m. UTC | #2
Hello Thomas,

Le jeu. 3 sept. 2020 à 22:47, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> a écrit :
>
> Hello Fabrice,
>
> On Sun, 30 Aug 2020 12:00:57 +0200
> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote:
>
> > host-gperf dependency was added in commit
> > 5d8926add5da1b0bdfb90a41f4d7f857864c5524 without any explanation in the
> > commit message but gperf can be disabled through BUILD_GPERF since
> > https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=3c22870c762f7925b5ff143d76f9affbade275ba
> >
> > So use this variable and drop this unneeded dependency
> >
> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>
> Thanks for looking into this. I agree that BUILD_GPERF=no will avoid
> the host-gperf dependency, however I'm a bit confused how this works in
> libcap and whether it has any drawback.
>
> When gperf is detected, it will be used to generate some C code that
> contains a hash function:
>
> ifeq ($(BUILD_GPERF),yes)
> USE_GPERF_OUTPUT = $(GPERF_OUTPUT)
> INCLUDE_GPERF_OUTPUT = -DINCLUDE_GPERF_OUTPUT='"$(GPERF_OUTPUT)"'
> endif
>
> $(GPERF_OUTPUT): cap_names.list.h
>         perl -e 'print "struct __cap_token_s { const char *name; int index; };\n%{\nconst struct __cap_token_s *__cap_lookup_name(const char *, size_t);\n%}\n%%\n"; while ($$l = <>) { $$l =~ s/[\{\"]//g; $$l =~ s/\}.*// ; print $$l; }' < $< | gperf --ignore-case --language=ANSI-C --readonly --null-strings --global-table --hash-function-name=__cap_hash_name --lookup-function-name="__cap_lookup_name" -c -t -m20 $(INDENT) > $@
>         sed -e 's/unsigned int len/size_t len/' -i $@
>
> cap_text.o: cap_text.c $(USE_GPERF_OUTPUT) $(INCLS)
>         $(CC) $(CFLAGS) $(IPATH) $(INCLUDE_GPERF_OUTPUT) -c $< -o $@
>
> Then in cap_text.c, this gets used like this:
>
> #ifdef INCLUDE_GPERF_OUTPUT
> /* we need to include it after #define _GNU_SOURCE is set */
> #include INCLUDE_GPERF_OUTPUT
> #endif
>
> So the gperf-generated source file, if it exists is included. But I
> don't see any conditional code that makes use of it when available.
gperf code has been added with
https://git.kernel.org/pub/scm/linux/kernel/git/morgan/libcap.git/commit/?id=e57378c88b6144ff9c06777ff0e0c9d722eeefd3

From my understanding, gperf code is used in libcap/cap_text.c by the
lookupname function:

#ifdef GPERF_DOWNCASE
 const struct __cap_token_s *token_info;
 int c;
 unsigned len;

 for (len=0; (c = str.constp[len]); ++len) {
 if (!(isalpha(c) || (c == '_'))) {
 break;
 }
 }
 token_info = __cap_lookup_name(str.constp, len);
 if (token_info != NULL) {
 *strp = str.constp + len;
 return token_info->index;
 }
#else /* ie., ndef GPERF_DOWNCASE */

lookupname is used by cap_from_name which is a part of the public API
and is also used by capsh program.
>
> Am I missing something? I wanted to understand the impact of not having
> gperf. Perhaps not having a hash function makes libcap significantly
> slower ?
Yes, you're right, not having gperf will have an unknown (but probably
small?) performance impact.
I didn't take time to measure it on an embedded target but we should
probably keep building host-gperf for the target (and probably also
for the host as host-gperf is not a "huge" dependency).
>
> Initially, I was expected libcap to have a pre-generated version of the
> file, and having gperf available would only allow to re-generate the
> file, but that doesn't seem to be what's happening.
>
> Do you understand a bit better what is going on here ?
>
> Thanks,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Best Regards,

Fabrice
diff mbox series

Patch

diff --git a/package/libcap/libcap.mk b/package/libcap/libcap.mk
index 08c1bc9deb..208f8eade1 100644
--- a/package/libcap/libcap.mk
+++ b/package/libcap/libcap.mk
@@ -10,11 +10,9 @@  LIBCAP_SOURCE = libcap-$(LIBCAP_VERSION).tar.xz
 LIBCAP_LICENSE = GPL-2.0 or BSD-3-Clause
 LIBCAP_LICENSE_FILES = License
 
-LIBCAP_DEPENDENCIES = host-libcap host-gperf
+LIBCAP_DEPENDENCIES = host-libcap
 LIBCAP_INSTALL_STAGING = YES
 
-HOST_LIBCAP_DEPENDENCIES = host-gperf
-
 ifeq ($(BR2_STATIC_LIBS),y)
 LIBCAP_MAKE_TARGET = libcap.a libcap.pc
 LIBCAP_MAKE_INSTALL_TARGET = install-static
@@ -28,7 +26,8 @@  endif
 
 LIBCAP_MAKE_FLAGS = \
 	BUILD_CC="$(HOSTCC)" \
-	BUILD_CFLAGS="$(HOST_CFLAGS)"
+	BUILD_CFLAGS="$(HOST_CFLAGS)" \
+	BUILD_GPERF=no
 
 ifeq ($(BR2_PACKAGE_LIBCAP_TOOLS),y)
 define LIBCAP_BUILD_TOOLS_CMDS
@@ -62,12 +61,12 @@  endef
 
 define HOST_LIBCAP_BUILD_CMDS
 	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) -C $(@D)\
-		RAISE_SETFCAP=no
+		BUILD_GPERF=no RAISE_SETFCAP=no
 endef
 
 define HOST_LIBCAP_INSTALL_CMDS
 	$(HOST_MAKE_ENV) $(MAKE) -C $(@D) prefix=$(HOST_DIR) \
-		RAISE_SETFCAP=no lib=lib install
+		BUILD_GPERF=no RAISE_SETFCAP=no lib=lib install
 endef
 
 $(eval $(generic-package))