Re: [Qemu-devel] [PATCH] seccomp: Fix build

[Fam Zheng <famz@redhat.com>]

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