* Re: [Qemu-devel] [PATCH] seccomp: Fix build
2017-09-18 8:31 ` Peter Maydell
@ 2017-09-18 12:30 ` Fam Zheng
0 siblings, 0 replies; 3+ messages in thread
From: Fam Zheng @ 2017-09-18 12:30 UTC (permalink / raw)
To: Peter Maydell; +Cc: Jan Kiszka, Eduardo Otubo, qemu-devel
On Mon, 09/18 09:31, Peter Maydell wrote:
> On 18 September 2017 at 08:46, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> >
> > vl.c includes seccomp.h, thus requires the related CFLAGS as well.
> >
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> > Makefile.objs | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile.objs b/Makefile.objs
> > index d9cf7ad791..4f1488d65d 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -61,7 +61,7 @@ bt-host.o-cflags := $(BLUEZ_CFLAGS)
> >
> > common-obj-y += dma-helpers.o
> > common-obj-y += vl.o
> > -vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
> > +vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS) $(SECCOMP_CFLAGS)
> > common-obj-y += tpm.o
> >
> > common-obj-$(CONFIG_SLIRP) += slirp/
>
> ...I get the feeling we're going to get a whole bunch of these
> compile errors and fixups as we switch over to trying to specify
> package-dependent cflags only on certain source files. Is
> there anything we can do to make this changeover more reliable?
I think we should spot those risky headers and clean them up. See below.
>
> In particular I think that just adding a new #include line
> to a source file shouldn't suddenly require us to change
> the makefile to add a FOO_CFLAGS if we can avoid it. If
> it is necessary, then we need to make sure that's a compile
> error on all platforms and configurations (including those
> where pkg-config happens to set FOO_CFLAGS to the empty string.)
My apologies in advance that I'm not directly answering your question, because I
don't know. Anyway.. IMHO being prudent on which library headers to pull in
from a source file is a good thing, so I feel the conversion a worthwhile
effort. It also forces developers to fully hide the implementation details
behind an internal API:
> qemu-foo.h:
void qemu_foo_do_a(void);
void qemu_foo_do_b(void);
> qemu-foo-internal.h:
/* A header that includes the library header, if necessary */
#include <libfoo.h>
#include "qemu-foo.h"
> qemu-foo-a.c:
#include "qemu-foo.h"
#include "qemu-foo-internal.h"
void qemu_foo_do_a(void)
{
...
}
> qemu-foo-b.c:
#include "qemu-foo.h"
#include "qemu-foo-internal.h"
void qemu_foo_do_b(void)
{
...
}
So in general we could clean up non-internal headers (as in the above example
the qemu-foo.h versus qemu-foo-internal.h) so that the library headers are not
included there, whenever possible.
If a library header is indeed wanted by different .c files, it probably should
goto QEMU_CFLAGS, like the glib headers.
But if a .c file does need to include a library header, it probably also wants
the libs flag as well. So, commonly -cflags and -libs still go together.
In the case of this patch, I think we should do this:
diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h
index e67c2dc840..9b092aa23f 100644
--- a/include/sysemu/seccomp.h
+++ b/include/sysemu/seccomp.h
@@ -21,7 +21,5 @@
#define QEMU_SECCOMP_SET_SPAWN (1 << 3)
#define QEMU_SECCOMP_SET_RESOURCECTL (1 << 4)
-#include <seccomp.h>
-
int seccomp_start(uint32_t seccomp_opts);
#endif
^ permalink raw reply related [flat|nested] 3+ messages in thread