[Qemu-devel] [PATCH RESEND 00/15] typedefs: Remove scarcely used declarations

[Qemu-devel] [PATCH RESEND 00/15] typedefs: Remove scarcely used declarations

[Philippe Mathieu-Daudé]

Hi,

Since this file became a hot topic [*], I took previous cleanups
from last year and continue to clean it.

Regards,

Phil.

[*] "HACKING: Clarify the paragraph about typedefs"
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02132.html
[*] "Fix duplicated typedefs to be able to compile with Clang in gnu99 mode"
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02117.html

Philippe Mathieu-Daudé (15):
  hw/input/ps2: Remove PS2State from "qemu/typedefs.h"
  hw/pcmcia: Remove PCMCIACardState from "qemu/typedefs.h"
  hw/ssi: Remove SSIBus from "qemu/typedefs.h"
  hw/ide/ahci: Remove AllwinnerAHCIState from "qemu/typedefs.h"
  hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h"
  hw/i2c/smbus: Remove SMBusDevice from "qemu/typedefs.h"
  hw/bt: Remove HCIInfo from "qemu/typedefs.h"
  hw/char/serial: Remove SerialState from "qemu/typedefs.h"
  hw/i386: Remove PCMachineClass from "qemu/typedefs.h"
  range: Remove Range from "qemu/typedefs.h"
  audio: Remove AudioState from "qemu/typedefs.h"
  ui/console: Remove MouseTransformInfo from qemu/typedefs.h
  ui/console: Remove QemuDmaBuf from "qemu/typedefs.h"
  ui/console: Remove DisplayState/DisplaySurface from "qemu/typedefs.h"
  RFC ui/console: Add "ui/pixelformat.h" to declare PixelFormat

 audio/audio_int.h         |  4 ++--
 hw/arm/strongarm.h        |  1 +
 include/hw/acpi/vmgenid.h |  1 +
 include/hw/arm/pxa.h      |  2 ++
 include/hw/arm/virt.h     |  1 +
 include/hw/char/serial.h  |  4 ++--
 include/hw/devices.h      |  1 +
 include/hw/i2c/smbus.h    |  2 ++
 include/hw/i386/pc.h      |  4 ++--
 include/hw/ide/ahci.h     |  4 ++--
 include/hw/input/ps2.h    |  2 ++
 include/hw/mem/nvdimm.h   |  1 +
 include/hw/nvram/fw_cfg.h |  8 +++++++-
 include/hw/pci-host/q35.h |  1 +
 include/hw/pci/pci.h      |  1 +
 include/hw/pcmcia.h       |  4 ++--
 include/hw/ssi/pl022.h    |  1 +
 include/hw/ssi/ssi.h      |  1 +
 include/qemu/range.h      |  4 ++--
 include/qemu/typedefs.h   | 19 -------------------
 include/sysemu/bt.h       |  4 ++--
 include/ui/console.h      | 25 +++++++++----------------
 include/ui/pixelformat.h  | 31 +++++++++++++++++++++++++++++++
 include/ui/qemu-pixman.h  |  2 ++
 24 files changed, 78 insertions(+), 50 deletions(-)
 create mode 100644 include/ui/pixelformat.h

-- 
2.17.2

[Qemu-devel] [PATCH 01/15] hw/input/ps2: Remove PS2State from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

PS2State is only used in "hw/input/ps2.h", there is no
need to expose it via "qemu/typedefs.h".

To clean "qemu/typedefs.h", move the forward declaration
to "hw/input/ps2.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/input/ps2.h  | 2 ++
 include/qemu/typedefs.h | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 213aa16aa3..b60455d4f6 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -31,6 +31,8 @@
 #define PS2_MOUSE_BUTTON_SIDE   0x08
 #define PS2_MOUSE_BUTTON_EXTRA  0x10
 
+typedef struct PS2State PS2State;
+
 /* ps2.c */
 void *ps2_kbd_init(void (*update_irq)(void *, int), void *update_arg);
 void *ps2_mouse_init(void (*update_irq)(void *, int), void *update_arg);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 741935fe36..28a8b9e7c2 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -83,7 +83,6 @@ typedef struct PixelFormat PixelFormat;
 typedef struct PostcopyDiscardState PostcopyDiscardState;
 typedef struct Property Property;
 typedef struct PropertyInfo PropertyInfo;
-typedef struct PS2State PS2State;
 typedef struct QBool QBool;
 typedef struct QDict QDict;
 typedef struct QEMUBH QEMUBH;
-- 
2.17.2

[Qemu-devel] [PATCH 02/15] hw/pcmcia: Remove PCMCIACardState from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

There is only one file requiring this typedef (hw/arm/pxa.h),
let it include "hw/pcmcia.h" directly to simplify "qemu/typedefs.h".

To clean "qemu/typedefs.h", move the declaration to "hw/pcmcia.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/pxa.h    | 1 +
 include/hw/pcmcia.h     | 4 ++--
 include/qemu/typedefs.h | 1 -
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
index 0df1199caa..f6dfb5c0cf 100644
--- a/include/hw/arm/pxa.h
+++ b/include/hw/arm/pxa.h
@@ -12,6 +12,7 @@
 
 #include "exec/memory.h"
 #include "target/arm/cpu-qom.h"
+#include "hw/pcmcia.h"
 
 /* Interrupt numbers */
 # define PXA2XX_PIC_SSP3	0
diff --git a/include/hw/pcmcia.h b/include/hw/pcmcia.h
index 79cac9c761..1b4080764f 100644
--- a/include/hw/pcmcia.h
+++ b/include/hw/pcmcia.h
@@ -18,13 +18,13 @@ typedef struct PCMCIASocket {
 #define PCMCIA_CARD_CLASS(cls) \
     OBJECT_CLASS_CHECK(PCMCIACardClass, cls, TYPE_PCMCIA_CARD)
 
-struct PCMCIACardState {
+typedef struct PCMCIACardState {
     /*< private >*/
     DeviceState parent_obj;
     /*< public >*/
 
     PCMCIASocket *slot;
-};
+} PCMCIACardState;
 
 typedef struct PCMCIACardClass {
     /*< private >*/
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 28a8b9e7c2..3bd9215d55 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -78,7 +78,6 @@ typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
 typedef struct PCIHostState PCIHostState;
 typedef struct PCMachineClass PCMachineClass;
 typedef struct PCMachineState PCMachineState;
-typedef struct PCMCIACardState PCMCIACardState;
 typedef struct PixelFormat PixelFormat;
 typedef struct PostcopyDiscardState PostcopyDiscardState;
 typedef struct Property Property;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 02/15] hw/pcmcia: Remove PCMCIACardState from "qemu/typedefs.h"

[Thomas Huth]

On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> There is only one file requiring this typedef (hw/arm/pxa.h),

I'd suggest to replace "file" with "header file" here.

Reviewed-by: Thomas Huth <thuth@redhat.com>

> let it include "hw/pcmcia.h" directly to simplify "qemu/typedefs.h".
> 
> To clean "qemu/typedefs.h", move the declaration to "hw/pcmcia.h"
> (removing the forward declaration).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/pxa.h    | 1 +
>  include/hw/pcmcia.h     | 4 ++--
>  include/qemu/typedefs.h | 1 -
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
> index 0df1199caa..f6dfb5c0cf 100644
> --- a/include/hw/arm/pxa.h
> +++ b/include/hw/arm/pxa.h
> @@ -12,6 +12,7 @@
>  
>  #include "exec/memory.h"
>  #include "target/arm/cpu-qom.h"
> +#include "hw/pcmcia.h"
>  
>  /* Interrupt numbers */
>  # define PXA2XX_PIC_SSP3	0
> diff --git a/include/hw/pcmcia.h b/include/hw/pcmcia.h
> index 79cac9c761..1b4080764f 100644
> --- a/include/hw/pcmcia.h
> +++ b/include/hw/pcmcia.h
> @@ -18,13 +18,13 @@ typedef struct PCMCIASocket {
>  #define PCMCIA_CARD_CLASS(cls) \
>      OBJECT_CLASS_CHECK(PCMCIACardClass, cls, TYPE_PCMCIA_CARD)
>  
> -struct PCMCIACardState {
> +typedef struct PCMCIACardState {
>      /*< private >*/
>      DeviceState parent_obj;
>      /*< public >*/
>  
>      PCMCIASocket *slot;
> -};
> +} PCMCIACardState;
>  
>  typedef struct PCMCIACardClass {
>      /*< private >*/
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 28a8b9e7c2..3bd9215d55 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -78,7 +78,6 @@ typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
>  typedef struct PCIHostState PCIHostState;
>  typedef struct PCMachineClass PCMachineClass;
>  typedef struct PCMachineState PCMachineState;
> -typedef struct PCMCIACardState PCMCIACardState;
>  typedef struct PixelFormat PixelFormat;
>  typedef struct PostcopyDiscardState PostcopyDiscardState;
>  typedef struct Property Property;
> 

[Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

There are only three files requiring this typedef, let them
include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h".

To clean "qemu/typedefs.h", move the forward declaration
to "hw/ssi/ssi.h".

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/strongarm.h      | 1 +
 include/hw/arm/pxa.h    | 1 +
 include/hw/ssi/pl022.h  | 1 +
 include/hw/ssi/ssi.h    | 1 +
 include/qemu/typedefs.h | 1 -
 5 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/strongarm.h b/hw/arm/strongarm.h
index e98840b461..ae51a1ae34 100644
--- a/hw/arm/strongarm.h
+++ b/hw/arm/strongarm.h
@@ -3,6 +3,7 @@
 
 #include "exec/memory.h"
 #include "target/arm/cpu-qom.h"
+#include "hw/ssi/ssi.h"
 
 #define SA_CS0          0x00000000
 #define SA_CS1          0x08000000
diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
index f6dfb5c0cf..f184349f02 100644
--- a/include/hw/arm/pxa.h
+++ b/include/hw/arm/pxa.h
@@ -13,6 +13,7 @@
 #include "exec/memory.h"
 #include "target/arm/cpu-qom.h"
 #include "hw/pcmcia.h"
+#include "hw/ssi/ssi.h"
 
 /* Interrupt numbers */
 # define PXA2XX_PIC_SSP3	0
diff --git a/include/hw/ssi/pl022.h b/include/hw/ssi/pl022.h
index a080519366..1cf16f1539 100644
--- a/include/hw/ssi/pl022.h
+++ b/include/hw/ssi/pl022.h
@@ -22,6 +22,7 @@
 #define HW_SSI_PL022_H
 
 #include "hw/sysbus.h"
+#include "hw/ssi/ssi.h"
 
 #define TYPE_PL022 "pl022"
 #define PL022(obj) OBJECT_CHECK(PL022State, (obj), TYPE_PL022)
diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 6a0c3c3cdb..bdbf3c51f5 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -13,6 +13,7 @@
 
 #include "hw/qdev.h"
 
+typedef struct SSIBus SSIBus;
 typedef struct SSISlave SSISlave;
 typedef struct SSISlaveClass SSISlaveClass;
 typedef enum SSICSMode SSICSMode;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 3bd9215d55..c026229573 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -108,7 +108,6 @@ typedef struct Range Range;
 typedef struct SerialState SerialState;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct SMBusDevice SMBusDevice;
-typedef struct SSIBus SSIBus;
 typedef struct uWireSlave uWireSlave;
 typedef struct VirtIODevice VirtIODevice;
 typedef struct Visitor Visitor;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Thomas Huth]

On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> There are only three files requiring this typedef, let them

s/files/header files/

Reviewed-by: Thomas Huth <thuth@redhat.com>

> include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h".
> 
> To clean "qemu/typedefs.h", move the forward declaration
> to "hw/ssi/ssi.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/strongarm.h      | 1 +
>  include/hw/arm/pxa.h    | 1 +
>  include/hw/ssi/pl022.h  | 1 +
>  include/hw/ssi/ssi.h    | 1 +
>  include/qemu/typedefs.h | 1 -
>  5 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/strongarm.h b/hw/arm/strongarm.h
> index e98840b461..ae51a1ae34 100644
> --- a/hw/arm/strongarm.h
> +++ b/hw/arm/strongarm.h
> @@ -3,6 +3,7 @@
>  
>  #include "exec/memory.h"
>  #include "target/arm/cpu-qom.h"
> +#include "hw/ssi/ssi.h"
>  
>  #define SA_CS0          0x00000000
>  #define SA_CS1          0x08000000
> diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
> index f6dfb5c0cf..f184349f02 100644
> --- a/include/hw/arm/pxa.h
> +++ b/include/hw/arm/pxa.h
> @@ -13,6 +13,7 @@
>  #include "exec/memory.h"
>  #include "target/arm/cpu-qom.h"
>  #include "hw/pcmcia.h"
> +#include "hw/ssi/ssi.h"
>  
>  /* Interrupt numbers */
>  # define PXA2XX_PIC_SSP3	0
> diff --git a/include/hw/ssi/pl022.h b/include/hw/ssi/pl022.h
> index a080519366..1cf16f1539 100644
> --- a/include/hw/ssi/pl022.h
> +++ b/include/hw/ssi/pl022.h
> @@ -22,6 +22,7 @@
>  #define HW_SSI_PL022_H
>  
>  #include "hw/sysbus.h"
> +#include "hw/ssi/ssi.h"
>  
>  #define TYPE_PL022 "pl022"
>  #define PL022(obj) OBJECT_CHECK(PL022State, (obj), TYPE_PL022)
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 6a0c3c3cdb..bdbf3c51f5 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -13,6 +13,7 @@
>  
>  #include "hw/qdev.h"
>  
> +typedef struct SSIBus SSIBus;
>  typedef struct SSISlave SSISlave;
>  typedef struct SSISlaveClass SSISlaveClass;
>  typedef enum SSICSMode SSICSMode;
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 3bd9215d55..c026229573 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -108,7 +108,6 @@ typedef struct Range Range;
>  typedef struct SerialState SerialState;
>  typedef struct SHPCDevice SHPCDevice;
>  typedef struct SMBusDevice SMBusDevice;
> -typedef struct SSIBus SSIBus;
>  typedef struct uWireSlave uWireSlave;
>  typedef struct VirtIODevice VirtIODevice;
>  typedef struct Visitor Visitor;
> 

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> There are only three files requiring this typedef, let them
> include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h".
>
> To clean "qemu/typedefs.h", move the forward declaration
> to "hw/ssi/ssi.h".
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/strongarm.h      | 1 +
>  include/hw/arm/pxa.h    | 1 +
>  include/hw/ssi/pl022.h  | 1 +
>  include/hw/ssi/ssi.h    | 1 +
>  include/qemu/typedefs.h | 1 -
>  5 files changed, 4 insertions(+), 1 deletion(-)

When typedefs.h changes, we recompile the world, but it pretty much only
ever changes when new typedefs are added.  Thus, *keeping* a typedef
there is therefore pretty cheap.

Nevertheless, we shouldn't keep typedefs there without a real reason.
Being able to move one away without having to add any new #include
directives is a strong sign for "no real reason".  I like patches doing
that.

What I don't like is adding #include directives just so you can move
typedefs out of typedefs.h: it slows down the build.  Granted, the four
added by this patch are a drop in the bucket.  The point I'm trying to
make is typedefs.h's purpose: it's for avoiding #include directives.
Circular ones in particular, but others, too.

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Thomas Huth]

On 2019-01-15 13:28, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> There are only three files requiring this typedef, let them
>> include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h".
>>
>> To clean "qemu/typedefs.h", move the forward declaration
>> to "hw/ssi/ssi.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/strongarm.h      | 1 +
>>  include/hw/arm/pxa.h    | 1 +
>>  include/hw/ssi/pl022.h  | 1 +
>>  include/hw/ssi/ssi.h    | 1 +
>>  include/qemu/typedefs.h | 1 -
>>  5 files changed, 4 insertions(+), 1 deletion(-)
> 
> When typedefs.h changes, we recompile the world, but it pretty much only
> ever changes when new typedefs are added.  Thus, *keeping* a typedef
> there is therefore pretty cheap.
> 
> Nevertheless, we shouldn't keep typedefs there without a real reason.
> Being able to move one away without having to add any new #include
> directives is a strong sign for "no real reason".  I like patches doing
> that.
> 
> What I don't like is adding #include directives just so you can move
> typedefs out of typedefs.h: it slows down the build.  Granted, the four
> added by this patch are a drop in the bucket.  The point I'm trying to
> make is typedefs.h's purpose: it's for avoiding #include directives.
> Circular ones in particular, but others, too.

Yes, agreed, removing things from typedefs.h just to add lots of
#includes in other files is not really the best idea. I also dropped
this patch in v2 of my current PULL request because of this reason.
Phil, I suggest to simply drop this patch. What we maybe could do is to
split up typedefs.h per subsystem, so that we additionally have a
hw/arm/typedefs.h, hw/ppc/typedefs.h etc. in the end, then the
target-specific typedefs would not clutter the common qemu/typedefs.h
file anymore.

 Thomas

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Paolo Bonzini]

On 15/01/19 13:56, Thomas Huth wrote:
> Yes, agreed, removing things from typedefs.h just to add lots of
> #includes in other files is not really the best idea. I also dropped
> this patch in v2 of my current PULL request because of this reason.
> Phil, I suggest to simply drop this patch. What we maybe could do is to
> split up typedefs.h per subsystem, so that we additionally have a
> hw/arm/typedefs.h, hw/ppc/typedefs.h etc. in the end, then the
> target-specific typedefs would not clutter the common qemu/typedefs.h
> file anymore.

I disagree.  The *inclusions* of target-specific typedefs.h would then
clutter something.

For the (important, mind) case of circular includes, allowing struct in
prototypes pretty much solves the issues, and a subsystem-specific
typedefs.h is another solution according to maintainer's discretion.

In this case, however, keeping subsystems self-contained is a good
reason to apply the patch.  Having SSIBus in typedefs.h means that the
next SSI type has a higher chance of being added to typedefs.h even if
it's not necessary.

Sometimes we need to take patches that seem unnecessary in order to keep
the codebase tidy.  Think of the churn that was the introduction of
hw/SUBSYSTEM.  It was painful and it added all the complexity to the
Makefiles in order to support recursive Makefile.objs in our
not-that-recursive makefiles.  However, it made MAINTAINERS usable and
now, a few years later, few of us would probably be happy to go back to
the flat hw/ directory.

Paolo

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/01/19 13:56, Thomas Huth wrote:
>> Yes, agreed, removing things from typedefs.h just to add lots of
>> #includes in other files is not really the best idea. I also dropped
>> this patch in v2 of my current PULL request because of this reason.
>> Phil, I suggest to simply drop this patch. What we maybe could do is to
>> split up typedefs.h per subsystem, so that we additionally have a
>> hw/arm/typedefs.h, hw/ppc/typedefs.h etc. in the end, then the
>> target-specific typedefs would not clutter the common qemu/typedefs.h
>> file anymore.
>
> I disagree.  The *inclusions* of target-specific typedefs.h would then
> clutter something.
>
> For the (important, mind) case of circular includes, allowing struct in
> prototypes pretty much solves the issues, and a subsystem-specific
> typedefs.h is another solution according to maintainer's discretion.
>
> In this case, however, keeping subsystems self-contained is a good
> reason to apply the patch.  Having SSIBus in typedefs.h means that the
> next SSI type has a higher chance of being added to typedefs.h even if
> it's not necessary.

typedefs.h churn appars to be a non-problem:

    $ git-log --oneline --since 'one year ago' include/qemu/typedefs.h 
    a98c370c46 typedefs: (Re-)sort entries alphabetically
    aec90730fb numa: Match struct to typedef name
    7cfda775e5 move ObjectClass to typedefs.h
    ea134caa08 typedefs: add QJSON
    201376cb9e typedefs: Remove PcGuestInfo from qemu/typedefs.h
    9f5c734d59 Typedef the subtypes of QObject in qemu/typedefs.h, too
    e70372fcaf lockable: add QemuLockable

> Sometimes we need to take patches that seem unnecessary in order to keep
> the codebase tidy.  Think of the churn that was the introduction of
> hw/SUBSYSTEM.  It was painful and it added all the complexity to the
> Makefiles in order to support recursive Makefile.objs in our
> not-that-recursive makefiles.  However, it made MAINTAINERS usable and
> now, a few years later, few of us would probably be happy to go back to
> the flat hw/ directory.

What problem exactly are we trying to solve here?

To the best of my knowledge, typedefs.h has been working just fine for
us, and I can't see why it shouldn't continue to work just fine.

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Paolo Bonzini]

On 16/01/19 09:33, Markus Armbruster wrote:
> What problem exactly are we trying to solve here?
> To the best of my knowledge, typedefs.h has been working just fine for
> us, and I can't see why it shouldn't continue to work just fine.

Patches don't have to solve problems.  They can just help long-term
maintainability, highlight code smells, etc.  Nobody is saying
typedefs.h doesn't work.  But it's a tool, and including headers from
headers is also a tool.  Each tool has its purpose and it is useful to
question what are the exact purposes of the tools.

typedefs.h is useful to avoid rebuilding the world too often if a type
is used many times as a pointer, but rarely as a struct and rarely has
functions called on its instances.  This is already a relatively rare
case, but here we're talking about files that are included less than 20
times; many of which also already include hw/ssi/ssi.h for their own
business.  So we're hardly rebuilding anything more often, also because
hw/ssi/ssi.h is not really changing fast.

typedefs.h is also useful to avoid circular header inclusions.  Here
hw/ssi/ssi.h is clearly a toplevel include for the SSI subsystem.  I
agree that includes in a headers _can_ be painful, but sometimes they
also tell you a hierarchy of what uses what.  typedefs.h doesn't;
forward struct declarations also do, and I all but dislike using them in
headers (Thomas, can you send v2 of the HACKING patch?).

Therefore, typedefs.h is not particularly useful for SSIBus.  It doesn't
hurt much, if at all, to leave it in.  On the other hand, it also
doesn't hurt much if at all to remove it; it makes the SSI code a very
tiny little bit more self contained.

It may be that it's not particularly useful just because SSI is small;
for example I2CBus is also in typedefs.h and it's used a lot more, maybe
one day SSIBus will be the same.  In doubt, let's drop the patch---but I
think it's useful to have the discussion and there are cases that are
not controversial at all in Philippe's series.

Paolo

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Gerd Hoffmann]

  Hi,

> typedefs.h is useful to avoid rebuilding the world too often if a type
> is used many times as a pointer, but rarely as a struct and rarely has
> functions called on its instances.

Related:  Can also be used to keep struct content private.  struct
QemuConsole for example is private to ui/console.c, but pointers to
QemuConsole are passed around alot in ui/* and hw/display/* code.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Paolo Bonzini]

On 16/01/19 12:34, Gerd Hoffmann wrote:
>   Hi,
> 
>> typedefs.h is useful to avoid rebuilding the world too often if a type
>> is used many times as a pointer, but rarely as a struct and rarely has
>> functions called on its instances.
> 
> Related:  Can also be used to keep struct content private.  struct
> QemuConsole for example is private to ui/console.c, but pointers to
> QemuConsole are passed around alot in ui/* and hw/display/* code.

True, though as we switch more and more from pointers to embedded
structs that does not work that much anymore.  Another way to do that is
to split the header in include/path/to/foo.h and path/to/foo_internal.h.

Paolo

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Michael S. Tsirkin]

On Wed, Jan 16, 2019 at 12:49:07PM +0100, Paolo Bonzini wrote:
> On 16/01/19 12:34, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >> typedefs.h is useful to avoid rebuilding the world too often if a type
> >> is used many times as a pointer, but rarely as a struct and rarely has
> >> functions called on its instances.
> > 
> > Related:  Can also be used to keep struct content private.  struct
> > QemuConsole for example is private to ui/console.c, but pointers to
> > QemuConsole are passed around alot in ui/* and hw/display/* code.
> 
> True, though as we switch more and more from pointers to embedded
> structs that does not work that much anymore.  Another way to do that is
> to split the header in include/path/to/foo.h and path/to/foo_internal.h.
> 
> Paolo

Not sure this will help since no tool checks structure isn't
actually used even though it's in internal.

If you want to go overboard it's solvable of course, e.g.
something like this will work:


E.g. in virtio.h

#ifndef VIRTIO_PRIVATE
#define VIRTIO_PRIVATE(f) (VIRTIO_PIVATE_##f)
#endif

struct VirtioPrivate {
	int VIRTIO_PRIVATE(bar);
};

and in virtio.c:

#define VIRTIO_PRIVATE(f) (f)
#include <virtio.h>


-- 
MST

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Jan 16, 2019 at 12:49:07PM +0100, Paolo Bonzini wrote:
>> On 16/01/19 12:34, Gerd Hoffmann wrote:
>> >   Hi,
>> > 
>> >> typedefs.h is useful to avoid rebuilding the world too often if a type
>> >> is used many times as a pointer, but rarely as a struct and rarely has
>> >> functions called on its instances.
>> > 
>> > Related:  Can also be used to keep struct content private.  struct
>> > QemuConsole for example is private to ui/console.c, but pointers to
>> > QemuConsole are passed around alot in ui/* and hw/display/* code.
>> 
>> True, though as we switch more and more from pointers to embedded
>> structs that does not work that much anymore.  Another way to do that is
>> to split the header in include/path/to/foo.h and path/to/foo_internal.h.
>> 
>> Paolo
>
> Not sure this will help since no tool checks structure isn't
> actually used even though it's in internal.
>
> If you want to go overboard it's solvable of course, e.g.
> something like this will work:
>
>
> E.g. in virtio.h
>
> #ifndef VIRTIO_PRIVATE
> #define VIRTIO_PRIVATE(f) (VIRTIO_PIVATE_##f)
> #endif
>
> struct VirtioPrivate {
> 	int VIRTIO_PRIVATE(bar);
> };
>
> and in virtio.c:
>
> #define VIRTIO_PRIVATE(f) (f)
> #include <virtio.h>

I don't like such games at all.

Instead, I'd recommend to keep internal headers out of include/, and
rely on review / grep to catch, correct and prevent inappropriate uses.

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 16/01/19 09:33, Markus Armbruster wrote:
>> What problem exactly are we trying to solve here?
>> To the best of my knowledge, typedefs.h has been working just fine for
>> us, and I can't see why it shouldn't continue to work just fine.
>
> Patches don't have to solve problems.  They can just help long-term
> maintainability, highlight code smells, etc.  Nobody is saying
> typedefs.h doesn't work.  But it's a tool, and including headers from
> headers is also a tool.  Each tool has its purpose and it is useful to
> question what are the exact purposes of the tools.
>
> typedefs.h is useful to avoid rebuilding the world too often if a type
> is used many times as a pointer, but rarely as a struct and rarely has
> functions called on its instances.  This is already a relatively rare
> case, but here we're talking about files that are included less than 20
> times; many of which also already include hw/ssi/ssi.h for their own
> business.  So we're hardly rebuilding anything more often, also because
> hw/ssi/ssi.h is not really changing fast.
>
> typedefs.h is also useful to avoid circular header inclusions.  Here
> hw/ssi/ssi.h is clearly a toplevel include for the SSI subsystem.  I
> agree that includes in a headers _can_ be painful, but sometimes they
> also tell you a hierarchy of what uses what.  typedefs.h doesn't;
> forward struct declarations also do, and I all but dislike using them in
> headers (Thomas, can you send v2 of the HACKING patch?).
>
> Therefore, typedefs.h is not particularly useful for SSIBus.  It doesn't
> hurt much, if at all, to leave it in.  On the other hand, it also
> doesn't hurt much if at all to remove it; it makes the SSI code a very
> tiny little bit more self contained.
>
> It may be that it's not particularly useful just because SSI is small;
> for example I2CBus is also in typedefs.h and it's used a lot more, maybe
> one day SSIBus will be the same.  In doubt, let's drop the patch---but I
> think it's useful to have the discussion and there are cases that are
> not controversial at all in Philippe's series.

I fully agree having the discussion is useful, and I also like many of
the patches in Philippe's series.

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Paolo Bonzini]

On 15/01/19 13:28, Markus Armbruster wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/strongarm.h      | 1 +
>>  include/hw/arm/pxa.h    | 1 +
>>  include/hw/ssi/pl022.h  | 1 +
>>  include/hw/ssi/ssi.h    | 1 +
>>  include/qemu/typedefs.h | 1 -
>>  5 files changed, 4 insertions(+), 1 deletion(-)
> When typedefs.h changes, we recompile the world, but it pretty much only
> ever changes when new typedefs are added.  Thus, *keeping* a typedef
> there is therefore pretty cheap.
> 
> Nevertheless, we shouldn't keep typedefs there without a real reason.
> Being able to move one away without having to add any new #include
> directives is a strong sign for "no real reason".  I like patches doing
> that.
> 
> What I don't like is adding #include directives just so you can move
> typedefs out of typedefs.h: it slows down the build.  Granted, the four

(three - one added line is the typedef).

> added by this patch are a drop in the bucket.  The point I'm trying to
> make is typedefs.h's purpose: it's for avoiding #include directives.
> Circular ones in particular, but others, too.

In this case, adding ssi.h inclusions to SSI controllers seems to be a
feature, not a bug.

Paolo

Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/01/19 13:28, Markus Armbruster wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/arm/strongarm.h      | 1 +
>>>  include/hw/arm/pxa.h    | 1 +
>>>  include/hw/ssi/pl022.h  | 1 +
>>>  include/hw/ssi/ssi.h    | 1 +
>>>  include/qemu/typedefs.h | 1 -
>>>  5 files changed, 4 insertions(+), 1 deletion(-)
>> When typedefs.h changes, we recompile the world, but it pretty much only
>> ever changes when new typedefs are added.  Thus, *keeping* a typedef
>> there is therefore pretty cheap.
>> 
>> Nevertheless, we shouldn't keep typedefs there without a real reason.
>> Being able to move one away without having to add any new #include
>> directives is a strong sign for "no real reason".  I like patches doing
>> that.
>> 
>> What I don't like is adding #include directives just so you can move
>> typedefs out of typedefs.h: it slows down the build.  Granted, the four
>
> (three - one added line is the typedef).

Correct.

>> added by this patch are a drop in the bucket.  The point I'm trying to
>> make is typedefs.h's purpose: it's for avoiding #include directives.
>> Circular ones in particular, but others, too.
>
> In this case, adding ssi.h inclusions to SSI controllers seems to be a
> feature, not a bug.

Adding #include can be a necessity.  It can't be a feature any more than
"slowing down your compiles" could be one :)

I'm particularly wary of unnecessary #include in headers.

[Qemu-devel] [PATCH 04/15] hw/ide/ahci: Remove AllwinnerAHCIState from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Files requiring AllwinnerAHCIState already include "hw/ide/ahci.h".

To clean "qemu/typedefs.h", move the declaration to "hw/ide/ahci.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/ide/ahci.h   | 4 ++--
 include/qemu/typedefs.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index b7bb2b02d6..b44e3000cf 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -73,13 +73,13 @@ typedef struct SysbusAHCIState {
 #define ALLWINNER_AHCI_MMIO_OFF  0x80
 #define ALLWINNER_AHCI_MMIO_SIZE 0x80
 
-struct AllwinnerAHCIState {
+typedef struct AllwinnerAHCIState {
     /*< private >*/
     SysbusAHCIState parent_obj;
     /*< public >*/
 
     MemoryRegion mmio;
     uint32_t regs[ALLWINNER_AHCI_MMIO_SIZE/4];
-};
+} AllwinnerAHCIState;
 
 #endif /* HW_IDE_AHCI_H */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index c026229573..3a5ca74bd3 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -8,7 +8,6 @@
 typedef struct AdapterInfo AdapterInfo;
 typedef struct AddressSpace AddressSpace;
 typedef struct AioContext AioContext;
-typedef struct AllwinnerAHCIState AllwinnerAHCIState;
 typedef struct AudioState AudioState;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
 typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 04/15] hw/ide/ahci: Remove AllwinnerAHCIState from "qemu/typedefs.h"

[Thomas Huth]

On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Files requiring AllwinnerAHCIState already include "hw/ide/ahci.h".
> 
> To clean "qemu/typedefs.h", move the declaration to "hw/ide/ahci.h"
> (removing the forward declaration).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/ide/ahci.h   | 4 ++--
>  include/qemu/typedefs.h | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
> index b7bb2b02d6..b44e3000cf 100644
> --- a/include/hw/ide/ahci.h
> +++ b/include/hw/ide/ahci.h
> @@ -73,13 +73,13 @@ typedef struct SysbusAHCIState {
>  #define ALLWINNER_AHCI_MMIO_OFF  0x80
>  #define ALLWINNER_AHCI_MMIO_SIZE 0x80
>  
> -struct AllwinnerAHCIState {
> +typedef struct AllwinnerAHCIState {
>      /*< private >*/
>      SysbusAHCIState parent_obj;
>      /*< public >*/
>  
>      MemoryRegion mmio;
>      uint32_t regs[ALLWINNER_AHCI_MMIO_SIZE/4];
> -};
> +} AllwinnerAHCIState;
>  
>  #endif /* HW_IDE_AHCI_H */
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index c026229573..3a5ca74bd3 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -8,7 +8,6 @@
>  typedef struct AdapterInfo AdapterInfo;
>  typedef struct AddressSpace AddressSpace;
>  typedef struct AioContext AioContext;
> -typedef struct AllwinnerAHCIState AllwinnerAHCIState;
>  typedef struct AudioState AudioState;
>  typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
>  typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH 05/15] hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

There are only three files requiring these typedefs, let them
include "hw/nvram/fw_cfg.h" directly to simplify "qemu/typedefs.h".

To clean "qemu/typedefs.h", move the forward declarations
to "hw/nvram/fw_cfg.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/acpi/vmgenid.h | 1 +
 include/hw/arm/virt.h     | 1 +
 include/hw/mem/nvdimm.h   | 1 +
 include/hw/nvram/fw_cfg.h | 8 +++++++-
 include/qemu/typedefs.h   | 4 ----
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
index 38586ecbdf..be53de38f1 100644
--- a/include/hw/acpi/vmgenid.h
+++ b/include/hw/acpi/vmgenid.h
@@ -3,6 +3,7 @@
 
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/qdev.h"
+#include "hw/nvram/fw_cfg.h"
 #include "qemu/uuid.h"
 
 #define VMGENID_DEVICE           "vmgenid"
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 4cc57a7ef6..fef632dcf5 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -35,6 +35,7 @@
 #include "qemu/notify.h"
 #include "hw/boards.h"
 #include "hw/arm/arm.h"
+#include "hw/nvram/fw_cfg.h"
 #include "sysemu/kvm.h"
 #include "hw/intc/arm_gicv3_common.h"
 
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index c5c9b3c7f8..51d3a52c4b 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -24,6 +24,7 @@
 #define QEMU_NVDIMM_H
 
 #include "hw/mem/pc-dimm.h"
+#include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
 
 #define NVDIMM_DEBUG 0
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f5a6895a74..244ed78afa 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -14,7 +14,9 @@
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
-typedef struct fw_cfg_file FWCfgFile;
+typedef struct FWCfgState FWCfgState;
+typedef struct FWCfgIoState FWCfgIoState;
+typedef struct FWCfgMemState FWCfgMemState;
 
 #define FW_CFG_ORDER_OVERRIDE_VGA    70
 #define FW_CFG_ORDER_OVERRIDE_NIC    80
@@ -24,6 +26,8 @@ typedef struct fw_cfg_file FWCfgFile;
 void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order);
 void fw_cfg_reset_order_override(FWCfgState *fw_cfg);
 
+typedef struct fw_cfg_file FWCfgFile;
+
 typedef struct FWCfgFiles {
     uint32_t  count;
     FWCfgFile f[];
@@ -34,6 +38,8 @@ typedef struct fw_cfg_dma_access FWCfgDmaAccess;
 typedef void (*FWCfgCallback)(void *opaque);
 typedef void (*FWCfgWriteCallback)(void *opaque, off_t start, size_t len);
 
+typedef struct FWCfgEntry FWCfgEntry;
+
 struct FWCfgState {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 3a5ca74bd3..1c294c296c 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -31,10 +31,6 @@ typedef struct DriveInfo DriveInfo;
 typedef struct Error Error;
 typedef struct EventNotifier EventNotifier;
 typedef struct FlatView FlatView;
-typedef struct FWCfgEntry FWCfgEntry;
-typedef struct FWCfgIoState FWCfgIoState;
-typedef struct FWCfgMemState FWCfgMemState;
-typedef struct FWCfgState FWCfgState;
 typedef struct HCIInfo HCIInfo;
 typedef struct HVFX86EmulatorState HVFX86EmulatorState;
 typedef struct I2CBus I2CBus;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 05/15] hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h"

[Laszlo Ersek]

On 01/11/19 15:08, Philippe Mathieu-Daudé wrote:
> There are only three files requiring these typedefs, let them
> include "hw/nvram/fw_cfg.h" directly to simplify "qemu/typedefs.h".
> 
> To clean "qemu/typedefs.h", move the forward declarations
> to "hw/nvram/fw_cfg.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/acpi/vmgenid.h | 1 +
>  include/hw/arm/virt.h     | 1 +
>  include/hw/mem/nvdimm.h   | 1 +
>  include/hw/nvram/fw_cfg.h | 8 +++++++-
>  include/qemu/typedefs.h   | 4 ----
>  5 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
> index 38586ecbdf..be53de38f1 100644
> --- a/include/hw/acpi/vmgenid.h
> +++ b/include/hw/acpi/vmgenid.h
> @@ -3,6 +3,7 @@
>  
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/qdev.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "qemu/uuid.h"
>  
>  #define VMGENID_DEVICE           "vmgenid"
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 4cc57a7ef6..fef632dcf5 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -35,6 +35,7 @@
>  #include "qemu/notify.h"
>  #include "hw/boards.h"
>  #include "hw/arm/arm.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "sysemu/kvm.h"
>  #include "hw/intc/arm_gicv3_common.h"
>  
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c5c9b3c7f8..51d3a52c4b 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -24,6 +24,7 @@
>  #define QEMU_NVDIMM_H
>  
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/nvram/fw_cfg.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  
>  #define NVDIMM_DEBUG 0
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..244ed78afa 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -14,7 +14,9 @@
>  #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>  #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>  
> -typedef struct fw_cfg_file FWCfgFile;
> +typedef struct FWCfgState FWCfgState;
> +typedef struct FWCfgIoState FWCfgIoState;
> +typedef struct FWCfgMemState FWCfgMemState;

I think the resultant file would be easier to read if you moved the
State typedefs before the spot where we make the first references to
them, namely in the FW_CFG*() function-like macros.

With that,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

>  
>  #define FW_CFG_ORDER_OVERRIDE_VGA    70
>  #define FW_CFG_ORDER_OVERRIDE_NIC    80
> @@ -24,6 +26,8 @@ typedef struct fw_cfg_file FWCfgFile;
>  void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order);
>  void fw_cfg_reset_order_override(FWCfgState *fw_cfg);
>  
> +typedef struct fw_cfg_file FWCfgFile;
> +
>  typedef struct FWCfgFiles {
>      uint32_t  count;
>      FWCfgFile f[];
> @@ -34,6 +38,8 @@ typedef struct fw_cfg_dma_access FWCfgDmaAccess;
>  typedef void (*FWCfgCallback)(void *opaque);
>  typedef void (*FWCfgWriteCallback)(void *opaque, off_t start, size_t len);
>  
> +typedef struct FWCfgEntry FWCfgEntry;
> +
>  struct FWCfgState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 3a5ca74bd3..1c294c296c 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -31,10 +31,6 @@ typedef struct DriveInfo DriveInfo;
>  typedef struct Error Error;
>  typedef struct EventNotifier EventNotifier;
>  typedef struct FlatView FlatView;
> -typedef struct FWCfgEntry FWCfgEntry;
> -typedef struct FWCfgIoState FWCfgIoState;
> -typedef struct FWCfgMemState FWCfgMemState;
> -typedef struct FWCfgState FWCfgState;
>  typedef struct HCIInfo HCIInfo;
>  typedef struct HVFX86EmulatorState HVFX86EmulatorState;
>  typedef struct I2CBus I2CBus;
> 

Re: [Qemu-devel] [PATCH 05/15] hw/nvram/fw_cfg: Remove various typedefs from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

On 1/11/19 5:26 PM, Laszlo Ersek wrote:
> On 01/11/19 15:08, Philippe Mathieu-Daudé wrote:
>> There are only three files requiring these typedefs, let them
>> include "hw/nvram/fw_cfg.h" directly to simplify "qemu/typedefs.h".
>>
>> To clean "qemu/typedefs.h", move the forward declarations
>> to "hw/nvram/fw_cfg.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/hw/acpi/vmgenid.h | 1 +
>>  include/hw/arm/virt.h     | 1 +
>>  include/hw/mem/nvdimm.h   | 1 +
>>  include/hw/nvram/fw_cfg.h | 8 +++++++-
>>  include/qemu/typedefs.h   | 4 ----
>>  5 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/acpi/vmgenid.h b/include/hw/acpi/vmgenid.h
>> index 38586ecbdf..be53de38f1 100644
>> --- a/include/hw/acpi/vmgenid.h
>> +++ b/include/hw/acpi/vmgenid.h
>> @@ -3,6 +3,7 @@
>>  
>>  #include "hw/acpi/bios-linker-loader.h"
>>  #include "hw/qdev.h"
>> +#include "hw/nvram/fw_cfg.h"
>>  #include "qemu/uuid.h"
>>  
>>  #define VMGENID_DEVICE           "vmgenid"
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 4cc57a7ef6..fef632dcf5 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -35,6 +35,7 @@
>>  #include "qemu/notify.h"
>>  #include "hw/boards.h"
>>  #include "hw/arm/arm.h"
>> +#include "hw/nvram/fw_cfg.h"
>>  #include "sysemu/kvm.h"
>>  #include "hw/intc/arm_gicv3_common.h"
>>  
>> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
>> index c5c9b3c7f8..51d3a52c4b 100644
>> --- a/include/hw/mem/nvdimm.h
>> +++ b/include/hw/mem/nvdimm.h
>> @@ -24,6 +24,7 @@
>>  #define QEMU_NVDIMM_H
>>  
>>  #include "hw/mem/pc-dimm.h"
>> +#include "hw/nvram/fw_cfg.h"
>>  #include "hw/acpi/bios-linker-loader.h"
>>  
>>  #define NVDIMM_DEBUG 0
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index f5a6895a74..244ed78afa 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -14,7 +14,9 @@
>>  #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
>>  #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
>>  
>> -typedef struct fw_cfg_file FWCfgFile;
>> +typedef struct FWCfgState FWCfgState;
>> +typedef struct FWCfgIoState FWCfgIoState;
>> +typedef struct FWCfgMemState FWCfgMemState;
> 
> I think the resultant file would be easier to read if you moved the
> State typedefs before the spot where we make the first references to
> them, namely in the FW_CFG*() function-like macros.

I'm glad you suggested that, that is the style I like and how I went
through first, but then changed to follow this file style. I'll update
happily.

> 
> With that,
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

> 
> Thanks
> Laszlo
> 
>>  
>>  #define FW_CFG_ORDER_OVERRIDE_VGA    70
>>  #define FW_CFG_ORDER_OVERRIDE_NIC    80
>> @@ -24,6 +26,8 @@ typedef struct fw_cfg_file FWCfgFile;
>>  void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order);
>>  void fw_cfg_reset_order_override(FWCfgState *fw_cfg);
>>  
>> +typedef struct fw_cfg_file FWCfgFile;
>> +
>>  typedef struct FWCfgFiles {
>>      uint32_t  count;
>>      FWCfgFile f[];
>> @@ -34,6 +38,8 @@ typedef struct fw_cfg_dma_access FWCfgDmaAccess;
>>  typedef void (*FWCfgCallback)(void *opaque);
>>  typedef void (*FWCfgWriteCallback)(void *opaque, off_t start, size_t len);
>>  
>> +typedef struct FWCfgEntry FWCfgEntry;
>> +
>>  struct FWCfgState {
>>      /*< private >*/
>>      SysBusDevice parent_obj;
>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>> index 3a5ca74bd3..1c294c296c 100644
>> --- a/include/qemu/typedefs.h
>> +++ b/include/qemu/typedefs.h
>> @@ -31,10 +31,6 @@ typedef struct DriveInfo DriveInfo;
>>  typedef struct Error Error;
>>  typedef struct EventNotifier EventNotifier;
>>  typedef struct FlatView FlatView;
>> -typedef struct FWCfgEntry FWCfgEntry;
>> -typedef struct FWCfgIoState FWCfgIoState;
>> -typedef struct FWCfgMemState FWCfgMemState;
>> -typedef struct FWCfgState FWCfgState;
>>  typedef struct HCIInfo HCIInfo;
>>  typedef struct HVFX86EmulatorState HVFX86EmulatorState;
>>  typedef struct I2CBus I2CBus;
>>
> 

[Qemu-devel] [PATCH 06/15] hw/i2c/smbus: Remove SMBusDevice from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring SMBusDevice already include "hw/i2c/smbus.h".

To clean "qemu/typedefs.h", move the forward declaration
to "hw/i2c/smbus.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i2c/smbus.h  | 2 ++
 include/qemu/typedefs.h | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index d8b1b9ee81..5c61c05999 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -35,6 +35,8 @@
 #define SMBUS_DEVICE_GET_CLASS(obj) \
      OBJECT_GET_CLASS(SMBusDeviceClass, (obj), TYPE_SMBUS_DEVICE)
 
+typedef struct SMBusDevice SMBusDevice;
+
 typedef struct SMBusDeviceClass
 {
     I2CSlaveClass parent_class;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 1c294c296c..4524e14d03 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -102,7 +102,6 @@ typedef struct RAMBlock RAMBlock;
 typedef struct Range Range;
 typedef struct SerialState SerialState;
 typedef struct SHPCDevice SHPCDevice;
-typedef struct SMBusDevice SMBusDevice;
 typedef struct uWireSlave uWireSlave;
 typedef struct VirtIODevice VirtIODevice;
 typedef struct Visitor Visitor;
-- 
2.17.2

[Qemu-devel] [PATCH 07/15] hw/bt: Remove HCIInfo from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring HCIInfo already include "sysemu/bt.h".

To clean "qemu/typedefs.h", move the declaration to "sysemu/bt.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/typedefs.h | 1 -
 include/sysemu/bt.h     | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 4524e14d03..92f9996107 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -31,7 +31,6 @@ typedef struct DriveInfo DriveInfo;
 typedef struct Error Error;
 typedef struct EventNotifier EventNotifier;
 typedef struct FlatView FlatView;
-typedef struct HCIInfo HCIInfo;
 typedef struct HVFX86EmulatorState HVFX86EmulatorState;
 typedef struct I2CBus I2CBus;
 typedef struct I2SCodec I2SCodec;
diff --git a/include/sysemu/bt.h b/include/sysemu/bt.h
index ddb05cd109..2fd8c0f14b 100644
--- a/include/sysemu/bt.h
+++ b/include/sysemu/bt.h
@@ -3,7 +3,7 @@
 
 /* BT HCI info */
 
-struct HCIInfo {
+typedef struct HCIInfo {
     int (*bdaddr_set)(struct HCIInfo *hci, const uint8_t *bd_addr);
     void (*cmd_send)(struct HCIInfo *hci, const uint8_t *data, int len);
     void (*sco_send)(struct HCIInfo *hci, const uint8_t *data, int len);
@@ -11,7 +11,7 @@ struct HCIInfo {
     void *opaque;
     void (*evt_recv)(void *opaque, const uint8_t *data, int len);
     void (*acl_recv)(void *opaque, const uint8_t *data, int len);
-};
+} HCIInfo;
 
 /* bt-host.c */
 struct HCIInfo *bt_host_hci(const char *id);
-- 
2.17.2

[Qemu-devel] [PATCH 08/15] hw/char/serial: Remove SerialState from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring SerialState already include "hw/char/serial.h".

To clean "qemu/typedefs.h", move the declaration to "hw/char/serial.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/char/serial.h | 4 ++--
 include/qemu/typedefs.h  | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 0acfbbc382..abd5cf71a9 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -35,7 +35,7 @@
 
 #define UART_FIFO_LENGTH    16      /* 16550A Fifo Length */
 
-struct SerialState {
+typedef struct SerialState {
     uint16_t divider;
     uint8_t rbr; /* receive register */
     uint8_t thr; /* transmit holding register */
@@ -77,7 +77,7 @@ struct SerialState {
 
     QEMUTimer *modem_status_poll;
     MemoryRegion io;
-};
+} SerialState;
 
 extern const VMStateDescription vmstate_serial;
 extern const MemoryRegionOps serial_io_ops;
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 92f9996107..165d335f87 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -99,7 +99,6 @@ typedef struct QObject QObject;
 typedef struct QString QString;
 typedef struct RAMBlock RAMBlock;
 typedef struct Range Range;
-typedef struct SerialState SerialState;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct uWireSlave uWireSlave;
 typedef struct VirtIODevice VirtIODevice;
-- 
2.17.2

[Qemu-devel] [PATCH 09/15] hw/i386: Remove PCMachineClass from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring PCMachineClass already include "hw/i386/pc.h".

To clean "qemu/typedefs.h", move the declaration to "hw/i386/pc.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/i386/pc.h    | 4 ++--
 include/qemu/typedefs.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 84720bede9..2c7a69b8f5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -96,7 +96,7 @@ struct PCMachineState {
  *                  way we can use 1GByte pages in the host.
  *
  */
-struct PCMachineClass {
+typedef struct PCMachineClass {
     /*< private >*/
     MachineClass parent_class;
 
@@ -133,7 +133,7 @@ struct PCMachineClass {
 
     /* use DMA capable linuxboot option rom */
     bool linuxboot_dma_enabled;
-};
+} PCMachineClass;
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
 #define PC_MACHINE(obj) \
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 165d335f87..5e9a070ff3 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -70,7 +70,6 @@ typedef struct PCIExpressDevice PCIExpressDevice;
 typedef struct PCIExpressHost PCIExpressHost;
 typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
 typedef struct PCIHostState PCIHostState;
-typedef struct PCMachineClass PCMachineClass;
 typedef struct PCMachineState PCMachineState;
 typedef struct PixelFormat PixelFormat;
 typedef struct PostcopyDiscardState PostcopyDiscardState;
-- 
2.17.2

[Qemu-devel] [PATCH 10/15] range: Remove Range from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Range is only used in "hw/pci-host/q35.h" in the MCHPCIState, and
in "hw/pci/pci.h" by pci_bus_get_w64_range().  Let them include
"qemu/range.h" directly to simplify "qemu/typedefs.h".

To clean "qemu/typedefs.h", move the declaration to "qemu/range.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/pci-host/q35.h | 1 +
 include/hw/pci/pci.h      | 1 +
 include/qemu/range.h      | 4 ++--
 include/qemu/typedefs.h   | 1 -
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 8f4ddde393..236ff008c9 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -22,6 +22,7 @@
 #ifndef HW_Q35_H
 #define HW_Q35_H
 
+#include "qemu/range.h"
 #include "hw/hw.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d87f5f93e9..f9c020fd98 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -1,6 +1,7 @@
 #ifndef QEMU_PCI_H
 #define QEMU_PCI_H
 
+#include "qemu/range.h"
 #include "hw/qdev.h"
 #include "exec/memory.h"
 #include "sysemu/dma.h"
diff --git a/include/qemu/range.h b/include/qemu/range.h
index 7e75f4e655..aaa09c8306 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -29,7 +29,7 @@
  * - Ranges must not wrap around 0, but can include UINT64_MAX.
  */
 
-struct Range {
+typedef struct Range {
     /*
      * Do not access members directly, use the functions!
      * A non-empty range has @lob <= @upb.
@@ -37,7 +37,7 @@ struct Range {
      */
     uint64_t lob;        /* inclusive lower bound */
     uint64_t upb;        /* inclusive upper bound */
-};
+} Range;
 
 static inline void range_invariant(const Range *range)
 {
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 5e9a070ff3..fb9f29d892 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -97,7 +97,6 @@ typedef struct QNum QNum;
 typedef struct QObject QObject;
 typedef struct QString QString;
 typedef struct RAMBlock RAMBlock;
-typedef struct Range Range;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct uWireSlave uWireSlave;
 typedef struct VirtIODevice VirtIODevice;
-- 
2.17.2

Re: [Qemu-devel] [PATCH 10/15] range: Remove Range from "qemu/typedefs.h"

[Thomas Huth]

On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
> Range is only used in "hw/pci-host/q35.h" in the MCHPCIState, and
> in "hw/pci/pci.h" by pci_bus_get_w64_range().  Let them include
> "qemu/range.h" directly to simplify "qemu/typedefs.h".
> 
> To clean "qemu/typedefs.h", move the declaration to "qemu/range.h"
> (removing the forward declaration).

Since Range seems to be meant as a common type (i.e. not specific to any
device like the other typedefs that you move in this series), I think
it's should be OK if this resides in typedefs.h ?

 Thomas

Re: [Qemu-devel] [PATCH 10/15] range: Remove Range from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

On 1/14/19 9:56 AM, Thomas Huth wrote:
> On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
>> Range is only used in "hw/pci-host/q35.h" in the MCHPCIState, and
>> in "hw/pci/pci.h" by pci_bus_get_w64_range().  Let them include
>> "qemu/range.h" directly to simplify "qemu/typedefs.h".
>>
>> To clean "qemu/typedefs.h", move the declaration to "qemu/range.h"
>> (removing the forward declaration).
> 
> Since Range seems to be meant as a common type (i.e. not specific to any
> device like the other typedefs that you move in this series), I think
> it's should be OK if this resides in typedefs.h ?

My idea was to clean files not very used, and this one is only used
twice. Why rebuild the World if someone add another function or fix a
typo in "qemu/range.h"?
I agree this typedef is meant for global use however.

Since I don't have a strong opinion, I'll drop it (until someone else
respond to this thread).

[Qemu-devel] [PATCH 11/15] audio: Remove AudioState from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring AudioState already include "audio_int.h".

To clean "qemu/typedefs.h", move the declaration to "audio_int.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 audio/audio_int.h       | 4 ++--
 include/qemu/typedefs.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/audio/audio_int.h b/audio/audio_int.h
index 244b454012..6c451b995c 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -191,7 +191,7 @@ struct SWVoiceCap {
     QLIST_ENTRY (SWVoiceCap) entries;
 };
 
-struct AudioState {
+typedef struct AudioState {
     struct audio_driver *drv;
     void *drv_opaque;
 
@@ -203,7 +203,7 @@ struct AudioState {
     int nb_hw_voices_out;
     int nb_hw_voices_in;
     int vm_running;
-};
+} AudioState;
 
 extern const struct mixeng_volume nominal_volume;
 
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index fb9f29d892..9baebe9d33 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -8,7 +8,6 @@
 typedef struct AdapterInfo AdapterInfo;
 typedef struct AddressSpace AddressSpace;
 typedef struct AioContext AioContext;
-typedef struct AudioState AudioState;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
 typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
 typedef struct BlockBackend BlockBackend;
-- 
2.17.2

[Qemu-devel] [PATCH 12/15] ui/console: Remove MouseTransformInfo from qemu/typedefs.h

[Philippe Mathieu-Daudé]

MouseTransformInfo is only used in "ui/console.h", there is no
need to expose it via "qemu/typedefs.h".

Move the declaration to "ui/console.h" (removing the forward
declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/devices.h    | 1 +
 include/qemu/typedefs.h | 1 -
 include/ui/console.h    | 4 ++--
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/devices.h b/include/hw/devices.h
index 0e27feb0c2..b5f1662225 100644
--- a/include/hw/devices.h
+++ b/include/hw/devices.h
@@ -4,6 +4,7 @@
 /* Devices that have nowhere better to go.  */
 
 #include "hw/hw.h"
+#include "ui/console.h"
 
 /* smc91c111.c */
 void smc91c111_init(NICInfo *, uint32_t, qemu_irq);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 9baebe9d33..e44f1473a5 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -49,7 +49,6 @@ typedef struct MigrationIncomingState MigrationIncomingState;
 typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
-typedef struct MouseTransformInfo MouseTransformInfo;
 typedef struct MSIMessage MSIMessage;
 typedef struct NetClientState NetClientState;
 typedef struct NetFilterState NetFilterState;
diff --git a/include/ui/console.h b/include/ui/console.h
index 853fcf4eb7..b74246f0f5 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -65,13 +65,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
 
 void kbd_put_ledstate(int ledstate);
 
-struct MouseTransformInfo {
+typedef struct MouseTransformInfo {
     /* Touchscreen resolution */
     int x;
     int y;
     /* Calibration values as used/generated by tslib */
     int a[7];
-};
+} MouseTransformInfo;
 
 void hmp_mouse_set(Monitor *mon, const QDict *qdict);
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH 12/15] ui/console: Remove MouseTransformInfo from qemu/typedefs.h

[Thomas Huth]

On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
> MouseTransformInfo is only used in "ui/console.h", there is no
> need to expose it via "qemu/typedefs.h".

You also touch devices.h here ... that should be mentioned, too?

 Thomas


> Move the declaration to "ui/console.h" (removing the forward
> declaration).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/devices.h    | 1 +
>  include/qemu/typedefs.h | 1 -
>  include/ui/console.h    | 4 ++--
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/devices.h b/include/hw/devices.h
> index 0e27feb0c2..b5f1662225 100644
> --- a/include/hw/devices.h
> +++ b/include/hw/devices.h
> @@ -4,6 +4,7 @@
>  /* Devices that have nowhere better to go.  */
>  
>  #include "hw/hw.h"
> +#include "ui/console.h"
>  
>  /* smc91c111.c */
>  void smc91c111_init(NICInfo *, uint32_t, qemu_irq);
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 9baebe9d33..e44f1473a5 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -49,7 +49,6 @@ typedef struct MigrationIncomingState MigrationIncomingState;
>  typedef struct MigrationState MigrationState;
>  typedef struct Monitor Monitor;
>  typedef struct MonitorDef MonitorDef;
> -typedef struct MouseTransformInfo MouseTransformInfo;
>  typedef struct MSIMessage MSIMessage;
>  typedef struct NetClientState NetClientState;
>  typedef struct NetFilterState NetFilterState;
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 853fcf4eb7..b74246f0f5 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -65,13 +65,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
>  
>  void kbd_put_ledstate(int ledstate);
>  
> -struct MouseTransformInfo {
> +typedef struct MouseTransformInfo {
>      /* Touchscreen resolution */
>      int x;
>      int y;
>      /* Calibration values as used/generated by tslib */
>      int a[7];
> -};
> +} MouseTransformInfo;
>  
>  void hmp_mouse_set(Monitor *mon, const QDict *qdict);
>  
> 

Re: [Qemu-devel] [PATCH 12/15] ui/console: Remove MouseTransformInfo from qemu/typedefs.h

[Philippe Mathieu-Daudé]

On 1/14/19 9:59 AM, Thomas Huth wrote:
> On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote:
>> MouseTransformInfo is only used in "ui/console.h", there is no
>> need to expose it via "qemu/typedefs.h".
> 
> You also touch devices.h here ... that should be mentioned, too?

It depends whichever of this series or 'remove "hw/devices.h"' enters
first :)

> 
>  Thomas
> 
> 
>> Move the declaration to "ui/console.h" (removing the forward
>> declaration).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/hw/devices.h    | 1 +
>>  include/qemu/typedefs.h | 1 -
>>  include/ui/console.h    | 4 ++--
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/devices.h b/include/hw/devices.h
>> index 0e27feb0c2..b5f1662225 100644
>> --- a/include/hw/devices.h
>> +++ b/include/hw/devices.h
>> @@ -4,6 +4,7 @@
>>  /* Devices that have nowhere better to go.  */
>>  
>>  #include "hw/hw.h"
>> +#include "ui/console.h"
>>  
>>  /* smc91c111.c */
>>  void smc91c111_init(NICInfo *, uint32_t, qemu_irq);
>> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
>> index 9baebe9d33..e44f1473a5 100644
>> --- a/include/qemu/typedefs.h
>> +++ b/include/qemu/typedefs.h
>> @@ -49,7 +49,6 @@ typedef struct MigrationIncomingState MigrationIncomingState;
>>  typedef struct MigrationState MigrationState;
>>  typedef struct Monitor Monitor;
>>  typedef struct MonitorDef MonitorDef;
>> -typedef struct MouseTransformInfo MouseTransformInfo;
>>  typedef struct MSIMessage MSIMessage;
>>  typedef struct NetClientState NetClientState;
>>  typedef struct NetFilterState NetFilterState;
>> diff --git a/include/ui/console.h b/include/ui/console.h
>> index 853fcf4eb7..b74246f0f5 100644
>> --- a/include/ui/console.h
>> +++ b/include/ui/console.h
>> @@ -65,13 +65,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
>>  
>>  void kbd_put_ledstate(int ledstate);
>>  
>> -struct MouseTransformInfo {
>> +typedef struct MouseTransformInfo {
>>      /* Touchscreen resolution */
>>      int x;
>>      int y;
>>      /* Calibration values as used/generated by tslib */
>>      int a[7];
>> -};
>> +} MouseTransformInfo;
>>  
>>  void hmp_mouse_set(Monitor *mon, const QDict *qdict);
>>  
>>
> 

[Qemu-devel] [PATCH 13/15] ui/console: Remove QemuDmaBuf from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring QemuDmaBuf already include "ui/console.h".

To clean "qemu/typedefs.h", move the declaration to "ui/console.h"
(removing the forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/typedefs.h | 1 -
 include/ui/console.h    | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index e44f1473a5..55f4de7259 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -77,7 +77,6 @@ typedef struct QBool QBool;
 typedef struct QDict QDict;
 typedef struct QEMUBH QEMUBH;
 typedef struct QemuConsole QemuConsole;
-typedef struct QemuDmaBuf QemuDmaBuf;
 typedef struct QEMUFile QEMUFile;
 typedef struct QemuLockable QemuLockable;
 typedef struct QemuMutex QemuMutex;
diff --git a/include/ui/console.h b/include/ui/console.h
index b74246f0f5..8396387c47 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -179,7 +179,7 @@ struct QEMUGLParams {
     int minor_ver;
 };
 
-struct QemuDmaBuf {
+typedef struct QemuDmaBuf {
     int       fd;
     uint32_t  width;
     uint32_t  height;
@@ -187,7 +187,7 @@ struct QemuDmaBuf {
     uint32_t  fourcc;
     uint32_t  texture;
     bool      y0_top;
-};
+} QemuDmaBuf;
 
 typedef struct DisplayChangeListenerOps {
     const char *dpy_name;
-- 
2.17.2

[Qemu-devel] [PATCH 14/15] ui/console: Remove DisplayState/DisplaySurface from "qemu/typedefs.h"

[Philippe Mathieu-Daudé]

Files requiring DisplayState/DisplaySurface already include "ui/console.h".

To clean "qemu/typedefs.h", move the declarations to "ui/console.h"
(removing DisplaySurface forward declaration).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qemu/typedefs.h | 2 --
 include/ui/console.h    | 6 ++++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 55f4de7259..af0dc6e065 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -24,8 +24,6 @@ typedef struct DeviceListener DeviceListener;
 typedef struct DeviceState DeviceState;
 typedef struct DirtyBitmapSnapshot DirtyBitmapSnapshot;
 typedef struct DisplayChangeListener DisplayChangeListener;
-typedef struct DisplayState DisplayState;
-typedef struct DisplaySurface DisplaySurface;
 typedef struct DriveInfo DriveInfo;
 typedef struct Error Error;
 typedef struct EventNotifier EventNotifier;
diff --git a/include/ui/console.h b/include/ui/console.h
index 8396387c47..aa9f975544 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -131,7 +131,7 @@ struct PixelFormat {
     uint8_t rbits, gbits, bbits, abits;
 };
 
-struct DisplaySurface {
+typedef struct DisplaySurface {
     pixman_format_code_t format;
     pixman_image_t *image;
     uint8_t flags;
@@ -140,7 +140,7 @@ struct DisplaySurface {
     GLenum gltype;
     GLuint texture;
 #endif
-};
+} DisplaySurface;
 
 typedef struct QemuUIInfo {
     /* geometry */
@@ -189,6 +189,8 @@ typedef struct QemuDmaBuf {
     bool      y0_top;
 } QemuDmaBuf;
 
+typedef struct DisplayState DisplayState;
+
 typedef struct DisplayChangeListenerOps {
     const char *dpy_name;
 
-- 
2.17.2

[Qemu-devel] [RFC PATCH 15/15] ui/console: Add "ui/pixelformat.h" to declare PixelFormat

[Philippe Mathieu-Daudé]

PixelFormat is used by "ui/console.h" and by "ui/qemu-pixman.h".

Create the new header "ui/pixelformat.h" to declare this structure,
and remove the forward declaration from "qemu/typedefs.h".

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC because "ui/console.h" has on license, so I added a default one.
---
 include/qemu/typedefs.h  |  1 -
 include/ui/console.h     | 11 +----------
 include/ui/pixelformat.h | 31 +++++++++++++++++++++++++++++++
 include/ui/qemu-pixman.h |  2 ++
 4 files changed, 34 insertions(+), 11 deletions(-)
 create mode 100644 include/ui/pixelformat.h

diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index af0dc6e065..b0f33aa065 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -67,7 +67,6 @@ typedef struct PCIExpressHost PCIExpressHost;
 typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
 typedef struct PCIHostState PCIHostState;
 typedef struct PCMachineState PCMachineState;
-typedef struct PixelFormat PixelFormat;
 typedef struct PostcopyDiscardState PostcopyDiscardState;
 typedef struct Property Property;
 typedef struct PropertyInfo PropertyInfo;
diff --git a/include/ui/console.h b/include/ui/console.h
index aa9f975544..4fa2fa5c47 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -2,6 +2,7 @@
 #define CONSOLE_H
 
 #include "ui/qemu-pixman.h"
+#include "ui/pixelformat.h"
 #include "qom/object.h"
 #include "qemu/notify.h"
 #include "qemu/error-report.h"
@@ -121,16 +122,6 @@ struct QemuConsoleClass {
 
 #define QEMU_ALLOCATED_FLAG     0x01
 
-struct PixelFormat {
-    uint8_t bits_per_pixel;
-    uint8_t bytes_per_pixel;
-    uint8_t depth; /* color depth in bits */
-    uint32_t rmask, gmask, bmask, amask;
-    uint8_t rshift, gshift, bshift, ashift;
-    uint8_t rmax, gmax, bmax, amax;
-    uint8_t rbits, gbits, bbits, abits;
-};
-
 typedef struct DisplaySurface {
     pixman_format_code_t format;
     pixman_image_t *image;
diff --git a/include/ui/pixelformat.h b/include/ui/pixelformat.h
new file mode 100644
index 0000000000..585097531f
--- /dev/null
+++ b/include/ui/pixelformat.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef QEMU_PIXELFORMAT_H
+#define QEMU_PIXELFORMAT_H
+
+typedef struct PixelFormat {
+    uint8_t bits_per_pixel;
+    uint8_t bytes_per_pixel;
+    uint8_t depth; /* color depth in bits */
+    uint32_t rmask, gmask, bmask, amask;
+    uint8_t rshift, gshift, bshift, ashift;
+    uint8_t rmax, gmax, bmax, amax;
+    uint8_t rbits, gbits, bbits, abits;
+} PixelFormat;
+
+#endif
diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index b7c82d17fc..999a4aa770 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -16,6 +16,8 @@
 #pragma GCC diagnostic pop
 #endif
 
+#include "ui/pixelformat.h"
+
 /*
  * pixman image formats are defined to be native endian,
  * that means host byte order on qemu.  So we go define
-- 
2.17.2

Re: [Qemu-devel] [RFC PATCH 15/15] ui/console: Add "ui/pixelformat.h" to declare PixelFormat

[Paolo Bonzini]

On 11/01/19 15:08, Philippe Mathieu-Daudé wrote:
> PixelFormat is used by "ui/console.h" and by "ui/qemu-pixman.h".

ui/console.h already includes ui/qemu-pixman.h, just move it there?

Paolo

> Create the new header "ui/pixelformat.h" to declare this structure,
> and remove the forward declaration from "qemu/typedefs.h".
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC because "ui/console.h" has on license, so I added a default one.

Re: [Qemu-devel] [RFC PATCH 15/15] ui/console: Add "ui/pixelformat.h" to declare PixelFormat

[Philippe Mathieu-Daudé]

On 1/11/19 6:32 PM, Paolo Bonzini wrote:
> On 11/01/19 15:08, Philippe Mathieu-Daudé wrote:
>> PixelFormat is used by "ui/console.h" and by "ui/qemu-pixman.h".
> 
> ui/console.h already includes ui/qemu-pixman.h, just move it there?

That was my first though but then noticed pixman seems a bit
different/unrelated. If Gerd is OK I will do it. (I should have written
that in the commit).

> 
> Paolo
> 
>> Create the new header "ui/pixelformat.h" to declare this structure,
>> and remove the forward declaration from "qemu/typedefs.h".
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> RFC because "ui/console.h" has on license, so I added a default one.
> 

Re: [Qemu-devel] [RFC PATCH 15/15] ui/console: Add "ui/pixelformat.h" to declare PixelFormat

[Gerd Hoffmann]

On Mon, Jan 14, 2019 at 11:24:14AM +0100, Philippe Mathieu-Daudé wrote:
> On 1/11/19 6:32 PM, Paolo Bonzini wrote:
> > On 11/01/19 15:08, Philippe Mathieu-Daudé wrote:
> >> PixelFormat is used by "ui/console.h" and by "ui/qemu-pixman.h".
> > 
> > ui/console.h already includes ui/qemu-pixman.h, just move it there?
> 
> That was my first though but then noticed pixman seems a bit
> different/unrelated. If Gerd is OK I will do it. (I should have written
> that in the commit).

console depends on pixman anyway, so that move would be fine.

BTW: Should not be that hard to remove PixelFormat altogether, or move
the bits which are still needed to vnc which is the only user left
(beside deprecated sdl1 which should go away soon).

cheers,
  Gerd

Re: [Qemu-devel] [RFC PATCH 15/15] ui/console: Add "ui/pixelformat.h" to declare PixelFormat

[Philippe Mathieu-Daudé]

On 1/14/19 1:35 PM, Gerd Hoffmann wrote:
> On Mon, Jan 14, 2019 at 11:24:14AM +0100, Philippe Mathieu-Daudé wrote:
>> On 1/11/19 6:32 PM, Paolo Bonzini wrote:
>>> On 11/01/19 15:08, Philippe Mathieu-Daudé wrote:
>>>> PixelFormat is used by "ui/console.h" and by "ui/qemu-pixman.h".
>>>
>>> ui/console.h already includes ui/qemu-pixman.h, just move it there?
>>
>> That was my first though but then noticed pixman seems a bit
>> different/unrelated. If Gerd is OK I will do it. (I should have written
>> that in the commit).
> 
> console depends on pixman anyway, so that move would be fine.

Good.

> 
> BTW: Should not be that hard to remove PixelFormat altogether, or move
> the bits which are still needed to vnc which is the only user left
> (beside deprecated sdl1 which should go away soon).

I can help there once SDL1 got removed.

Thanks,

Phil.