[Qemu-devel] [PATCH] Stop including qemu-common.h in memory.h

[Qemu-devel] [PATCH] Stop including qemu-common.h in memory.h

[Peter Maydell]

Including qemu-common.h from other header files is generally a bad
idea, because it means it's very easy to end up with a circular
dependency. For instance, if we wanted to include memory.h from
qom/cpu.h we'd end up with this loop:
 memory.h -> qemu-common.h -> cpu.h -> cpu-qom.h -> qom/cpu.h -> memory.h

Remove the include from memory.h. This requires us to fix up a few
other files which were inadvertently getting declarations indirectly
through memory.h.

The biggest change is splitting the fprintf_function typedef out
into its own header so other headers can get at it without having
to include qemu-common.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I originally wrote this because I thought I was going to need to
include memory.h in qom/cpu.h. In fact I managed to avoid that in
the end, but this still seems like useful cleanup.


 include/exec/cpu-common.h |  2 ++
 include/exec/memory.h     |  1 -
 include/hw/arm/arm.h      |  1 +
 include/qemu-common.h     |  4 +---
 include/qemu/fprintf-fn.h | 16 ++++++++++++++++
 target-s390x/mmu_helper.c |  2 +-
 6 files changed, 21 insertions(+), 5 deletions(-)
 create mode 100644 include/qemu/fprintf-fn.h

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index de8a720..9fb1d54 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -13,6 +13,8 @@
 
 #include "qemu/bswap.h"
 #include "qemu/queue.h"
+#include "qemu/fprintf-fn.h"
+#include "qemu/typedefs.h"
 
 /**
  * CPUListState:
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 8ae004e..1881320 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -23,7 +23,6 @@
 
 #include <stdint.h>
 #include <stdbool.h>
-#include "qemu-common.h"
 #include "exec/cpu-common.h"
 #ifndef CONFIG_USER_ONLY
 #include "exec/hwaddr.h"
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 760804c..4dcd4f9 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -14,6 +14,7 @@
 #include "exec/memory.h"
 #include "hw/irq.h"
 #include "qemu/notify.h"
+#include "cpu.h"
 
 /* armv7m.c */
 qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
diff --git a/include/qemu-common.h b/include/qemu-common.h
index d52d09c..09b8c06 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -15,6 +15,7 @@
 #include "qemu/compiler.h"
 #include "config-host.h"
 #include "qemu/typedefs.h"
+#include "qemu/fprintf-fn.h"
 
 #if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
 #define WORDS_ALIGNED
@@ -85,9 +86,6 @@
 # error Unknown pointer size
 #endif
 
-typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
-
 #ifdef _WIN32
 #define fsync _commit
 #if !defined(lseek)
diff --git a/include/qemu/fprintf-fn.h b/include/qemu/fprintf-fn.h
new file mode 100644
index 0000000..4f66d45
--- /dev/null
+++ b/include/qemu/fprintf-fn.h
@@ -0,0 +1,16 @@
+/*
+ * Typedef for fprintf-alike function pointers.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef FPRINTF_FN_H
+
+#include "qemu/compiler.h"
+#include <stdio.h>
+
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
+#endif
diff --git a/target-s390x/mmu_helper.c b/target-s390x/mmu_helper.c
index 815ff42..1ea6d81 100644
--- a/target-s390x/mmu_helper.c
+++ b/target-s390x/mmu_helper.c
@@ -17,8 +17,8 @@
 
 #include "qemu/error-report.h"
 #include "exec/address-spaces.h"
-#include "sysemu/kvm.h"
 #include "cpu.h"
+#include "sysemu/kvm.h"
 
 /* #define DEBUG_S390 */
 /* #define DEBUG_S390_PTE */
-- 
1.9.1

Re: [Qemu-devel] [PATCH] Stop including qemu-common.h in memory.h

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Including qemu-common.h from other header files is generally a bad
> idea, because it means it's very easy to end up with a circular
> dependency. For instance, if we wanted to include memory.h from
> qom/cpu.h we'd end up with this loop:
>  memory.h -> qemu-common.h -> cpu.h -> cpu-qom.h -> qom/cpu.h -> memory.h
>
> Remove the include from memory.h. This requires us to fix up a few
> other files which were inadvertently getting declarations indirectly
> through memory.h.
>
> The biggest change is splitting the fprintf_function typedef out
> into its own header so other headers can get at it without having
> to include qemu-common.h.

Why not simply put it in qemu/typedefs.h?

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I originally wrote this because I thought I was going to need to
> include memory.h in qom/cpu.h. In fact I managed to avoid that in
> the end, but this still seems like useful cleanup.

Yup.

Re: [Qemu-devel] [PATCH] Stop including qemu-common.h in memory.h

[Peter Maydell]

On 3 July 2015 at 18:26, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Including qemu-common.h from other header files is generally a bad
>> idea, because it means it's very easy to end up with a circular
>> dependency. For instance, if we wanted to include memory.h from
>> qom/cpu.h we'd end up with this loop:
>>  memory.h -> qemu-common.h -> cpu.h -> cpu-qom.h -> qom/cpu.h -> memory.h
>>
>> Remove the include from memory.h. This requires us to fix up a few
>> other files which were inadvertently getting declarations indirectly
>> through memory.h.
>>
>> The biggest change is splitting the fprintf_function typedef out
>> into its own header so other headers can get at it without having
>> to include qemu-common.h.
>
> Why not simply put it in qemu/typedefs.h?

I thought about that, but that header claims it is specifically for
opaque typedefs, and also I didn't really want to add the stdio.h
and compiler.h includes to it.

-- PMM

Re: [Qemu-devel] [PATCH] Stop including qemu-common.h in memory.h

[Paolo Bonzini]

On 03/07/2015 16:18, Peter Maydell wrote:
> Including qemu-common.h from other header files is generally a bad
> idea, because it means it's very easy to end up with a circular
> dependency. For instance, if we wanted to include memory.h from
> qom/cpu.h we'd end up with this loop:
>  memory.h -> qemu-common.h -> cpu.h -> cpu-qom.h -> qom/cpu.h -> memory.h
> 
> Remove the include from memory.h. This requires us to fix up a few
> other files which were inadvertently getting declarations indirectly
> through memory.h.
> 
> The biggest change is splitting the fprintf_function typedef out
> into its own header so other headers can get at it without having
> to include qemu-common.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I originally wrote this because I thought I was going to need to
> include memory.h in qom/cpu.h. In fact I managed to avoid that in
> the end, but this still seems like useful cleanup.
> 
> 
>  include/exec/cpu-common.h |  2 ++
>  include/exec/memory.h     |  1 -
>  include/hw/arm/arm.h      |  1 +
>  include/qemu-common.h     |  4 +---
>  include/qemu/fprintf-fn.h | 16 ++++++++++++++++
>  target-s390x/mmu_helper.c |  2 +-
>  6 files changed, 21 insertions(+), 5 deletions(-)
>  create mode 100644 include/qemu/fprintf-fn.h
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index de8a720..9fb1d54 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -13,6 +13,8 @@
>  
>  #include "qemu/bswap.h"
>  #include "qemu/queue.h"
> +#include "qemu/fprintf-fn.h"
> +#include "qemu/typedefs.h"
>  
>  /**
>   * CPUListState:
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 8ae004e..1881320 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -23,7 +23,6 @@
>  
>  #include <stdint.h>
>  #include <stdbool.h>
> -#include "qemu-common.h"
>  #include "exec/cpu-common.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "exec/hwaddr.h"
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 760804c..4dcd4f9 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -14,6 +14,7 @@
>  #include "exec/memory.h"
>  #include "hw/irq.h"
>  #include "qemu/notify.h"
> +#include "cpu.h"
>  
>  /* armv7m.c */
>  qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index d52d09c..09b8c06 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -15,6 +15,7 @@
>  #include "qemu/compiler.h"
>  #include "config-host.h"
>  #include "qemu/typedefs.h"
> +#include "qemu/fprintf-fn.h"
>  
>  #if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
>  #define WORDS_ALIGNED
> @@ -85,9 +86,6 @@
>  # error Unknown pointer size
>  #endif
>  
> -typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> -    GCC_FMT_ATTR(2, 3);
> -
>  #ifdef _WIN32
>  #define fsync _commit
>  #if !defined(lseek)
> diff --git a/include/qemu/fprintf-fn.h b/include/qemu/fprintf-fn.h
> new file mode 100644
> index 0000000..4f66d45
> --- /dev/null
> +++ b/include/qemu/fprintf-fn.h
> @@ -0,0 +1,16 @@
> +/*
> + * Typedef for fprintf-alike function pointers.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef FPRINTF_FN_H
> +
> +#include "qemu/compiler.h"
> +#include <stdio.h>
> +
> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +
> +#endif
> diff --git a/target-s390x/mmu_helper.c b/target-s390x/mmu_helper.c
> index 815ff42..1ea6d81 100644
> --- a/target-s390x/mmu_helper.c
> +++ b/target-s390x/mmu_helper.c
> @@ -17,8 +17,8 @@
>  
>  #include "qemu/error-report.h"
>  #include "exec/address-spaces.h"
> -#include "sysemu/kvm.h"
>  #include "cpu.h"
> +#include "sysemu/kvm.h"
>  
>  /* #define DEBUG_S390 */
>  /* #define DEBUG_S390_PTE */
> 

Applied for 2.4, thanks.

Paolo