| Message ID | 20190726120542.9894-28-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tame a few "touch this, recompile the world" headers | expand |
On 26/07/19 14:05, Markus Armbruster wrote: > +typedef struct VMChangeStateEntry VMChangeStateEntry; > typedef struct VMStateDescription VMStateDescription; > This is a bit borderline; I'd rather split sysemu/sysemu.h, e.g. adding sysemu/runstate.h that would have VMChangeStateEntry functions. If there aren't many conflicts, perhaps you can drop this patch? Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 26/07/19 14:05, Markus Armbruster wrote: >> +typedef struct VMChangeStateEntry VMChangeStateEntry; >> typedef struct VMStateDescription VMStateDescription; >> > > This is a bit borderline; I'd rather split sysemu/sysemu.h, e.g. adding > sysemu/runstate.h that would have VMChangeStateEntry functions. If > there aren't many conflicts, perhaps you can drop this patch? Without it, the next one will be ineffective. Which parts of sysemu.h would you rather move to runstate.h? * VMChangeStateEntry and the three functions using it, obviously, along with VMChangeStateHandler. * vm_state_notify(), because it belongs to the above. * The runstate_FOO() functions, because they're named like the new header? * vm_stop(), vm_stop_force_state(), vmstop_requested(), vmstop_requested, because they use RunState? * The remaining vm_FOO(), because they're closely related to vm_stop()? * Everything else from qemu_exit_preconfig_request() to qemu_remove_exit_notifier(), along with WakeupReason? * More? If only the first two, we can call the new header vmstate-notify.h.
On 02/08/19 15:16, Markus Armbruster wrote: > * VMChangeStateEntry and the three functions using it, obviously, along > with VMChangeStateHandler. > > * vm_state_notify(), because it belongs to the above. > > * The runstate_FOO() functions, because they're named like the new > header? > > * vm_stop(), vm_stop_force_state(), vmstop_requested(), > vmstop_requested, because they use RunState? > > * The remaining vm_FOO(), because they're closely related to vm_stop()? > > * Everything else from qemu_exit_preconfig_request() to > qemu_remove_exit_notifier(), along with WakeupReason? Yes, that could be an idea, but not qemu_add/remove_exit_notifier. Paolo
Markus Armbruster <armbru@redhat.com> writes: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 26/07/19 14:05, Markus Armbruster wrote: >>> +typedef struct VMChangeStateEntry VMChangeStateEntry; >>> typedef struct VMStateDescription VMStateDescription; >>> >> >> This is a bit borderline; I'd rather split sysemu/sysemu.h, e.g. adding >> sysemu/runstate.h that would have VMChangeStateEntry functions. If >> there aren't many conflicts, perhaps you can drop this patch? > > Without it, the next one will be ineffective. Actually, "ineffective" is an exaggeration, it's not *that* bad. Two headers are affected: Before 27 After 27+28 Just 28 qapi/qapi-types-run-state.h 5500 4400 5000 sysemu/vmstate-notify.h 0 180 1000 The numbers are .o depending on the header in my "build everything" tree, out of 6600 total (not counting tests and objects that don't depend on qemu/osdep.h). Mainly because hw/virtio/virtio.h (760 .o) and hw/scsi/scsi.h (180 .o) need to include sysemu/vmstate-notify.h to get the VMChangeStateEntry typedef. Four more headers need it as well, but they are small potatoes. Splitting sysemu/runstate.h off sysemu/sysemu.h makes sense anyway, but won't improve these numbers. I'd keep PATCH 27. [...]
Paolo Bonzini <pbonzini@redhat.com> writes: > On 02/08/19 15:16, Markus Armbruster wrote: >> * VMChangeStateEntry and the three functions using it, obviously, along >> with VMChangeStateHandler. >> >> * vm_state_notify(), because it belongs to the above. >> >> * The runstate_FOO() functions, because they're named like the new >> header? >> >> * vm_stop(), vm_stop_force_state(), vmstop_requested(), >> vmstop_requested, because they use RunState? >> >> * The remaining vm_FOO(), because they're closely related to vm_stop()? >> >> * Everything else from qemu_exit_preconfig_request() to >> qemu_remove_exit_notifier(), along with WakeupReason? > > Yes, that could be an idea, but not qemu_add/remove_exit_notifier. I did this in v2 this instead of creating sysemu/vmstate-notify.h. Reminder: * PATCH 27 "sysemu: Move the VMChangeStateEntry typedef to qemu/typedefs.h" makes the subsequent patches more effective: it saves ~800 dependencies on whatever header defines the typedef. * PATCH 28 "Include sysemu/sysemu.h a lot less" succeeds almost entirely due to not including it from hw/qdev-core.h. Damage done by recent commit e965ffa70a "qdev: add qdev_add_vm_change_state_handler()". The stupidest way to undo that damage would be moving qdev_add_vm_change_state_handler() to sysemu/sysemu.h. I chose to move it to new sysemu/vmstate-notify.h along with its buddies from sysemu/sysemu.h. The dependency improvement is negligible, just because it makes sysemu/sysemu.h less of a dumping ground. For v2, I flipped PATCH 28 to the stupidest way, and put it *before* PATCH 27. I split off sysemu/runstate.h only in PATCH 29. I hope that way the benefit of each change is more obvious.
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h index c6954c1d56..52ec197da0 100644 --- a/include/hw/ide/internal.h +++ b/include/hw/ide/internal.h @@ -6,11 +6,12 @@ * only files in hw/ide/ are supposed to include this file. * non-internal declarations are in hw/ide.h */ + +#include "qapi/qapi-types-run-state.h" #include "hw/ide.h" #include "hw/irq.h" #include "hw/isa/isa.h" #include "sysemu/dma.h" -#include "sysemu/sysemu.h" #include "hw/block/block.h" #include "scsi/constants.h" diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h index a39e672f27..bfd40f01d8 100644 --- a/include/hw/ppc/spapr_xive.h +++ b/include/hw/ppc/spapr_xive.h @@ -12,7 +12,6 @@ #include "hw/ppc/spapr_irq.h" #include "hw/ppc/xive.h" -#include "sysemu/sysemu.h" #define TYPE_SPAPR_XIVE "spapr-xive" #define SPAPR_XIVE(obj) OBJECT_CHECK(SpaprXive, (obj), TYPE_SPAPR_XIVE) diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h index 2bfaad0fe9..d77a92361b 100644 --- a/include/hw/scsi/scsi.h +++ b/include/hw/scsi/scsi.h @@ -4,7 +4,6 @@ #include "block/aio.h" #include "hw/block/block.h" #include "hw/qdev-core.h" -#include "sysemu/sysemu.h" #include "scsi/utils.h" #include "qemu/notify.h" diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index df40a46d60..48e8d04ff6 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -17,7 +17,6 @@ #include "exec/memory.h" #include "hw/qdev-core.h" #include "net/net.h" -#include "sysemu/sysemu.h" #include "migration/vmstate.h" #include "qemu/event_notifier.h" #include "standard-headers/linux/virtio_config.h" diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index f569f5f270..3fcdde8bfc 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -102,6 +102,7 @@ typedef struct SHPCDevice SHPCDevice; typedef struct SSIBus SSIBus; typedef struct VirtIODevice VirtIODevice; typedef struct Visitor Visitor; +typedef struct VMChangeStateEntry VMChangeStateEntry; typedef struct VMStateDescription VMStateDescription; /* diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 227202999d..7e445e0683 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -22,7 +22,6 @@ void runstate_set(RunState new_state); int runstate_is_running(void); bool runstate_needs_reset(void); bool runstate_store(char *str, size_t size); -typedef struct vm_change_state_entry VMChangeStateEntry; typedef void VMChangeStateHandler(void *opaque, int running, RunState state); VMChangeStateEntry *qemu_add_vm_change_state_handler(VMChangeStateHandler *cb, diff --git a/vl.c b/vl.c index df23dd6913..91255acf71 100644 --- a/vl.c +++ b/vl.c @@ -1362,14 +1362,14 @@ static int machine_help_func(QemuOpts *opts, MachineState *machine) return 1; } -struct vm_change_state_entry { +struct VMChangeStateEntry { VMChangeStateHandler *cb; void *opaque; - QTAILQ_ENTRY(vm_change_state_entry) entries; + QTAILQ_ENTRY(VMChangeStateEntry) entries; int priority; }; -static QTAILQ_HEAD(, vm_change_state_entry) vm_change_state_head; +static QTAILQ_HEAD(, VMChangeStateEntry) vm_change_state_head; /** * qemu_add_vm_change_state_handler_prio:
The previous commit deleted superfluous inclusions of sysemu/sysemu.h, but that didn't really help. Several headers include sysemu/sysemu.h just to get typedef VMChangeStateEntry. Move it from sysemu/sysemu.h to qemu/typedefs.h. Spell its structure tag the same while there. All these headers still include sysemu/sysemu.h indirectly, so this doesn't by itself reduce its use. It will together with the the next commit. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- include/hw/ide/internal.h | 3 ++- include/hw/ppc/spapr_xive.h | 1 - include/hw/scsi/scsi.h | 1 - include/hw/virtio/virtio.h | 1 - include/qemu/typedefs.h | 1 + include/sysemu/sysemu.h | 1 - vl.c | 6 +++--- 7 files changed, 6 insertions(+), 8 deletions(-)