| Message ID | 1714406135-451286-8-git-send-email-steven.sistare@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Live update: cpr-exec | expand |
Steve Sistare <steven.sistare@oracle.com> writes: > Define a type for the 256 byte id string to guarantee the same length is > used and enforced everywhere. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote: > Define a type for the 256 byte id string to guarantee the same length is > used and enforced everywhere. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > include/exec/ramblock.h | 3 ++- > include/migration/vmstate.h | 2 ++ > migration/savevm.c | 8 ++++---- > migration/vmstate.c | 3 ++- > 4 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h > index 0babd10..61deefe 100644 > --- a/include/exec/ramblock.h > +++ b/include/exec/ramblock.h > @@ -23,6 +23,7 @@ > #include "cpu-common.h" > #include "qemu/rcu.h" > #include "exec/ramlist.h" > +#include "migration/vmstate.h" > > struct RAMBlock { > struct rcu_head rcu; > @@ -35,7 +36,7 @@ struct RAMBlock { > void (*resized)(const char*, uint64_t length, void *host); > uint32_t flags; > /* Protected by the BQL. */ > - char idstr[256]; > + VMStateId idstr; > /* RCU-enabled, writes protected by the ramlist lock */ > QLIST_ENTRY(RAMBlock) next; > QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; Hmm.. Don't look like a good idea to include a migration header in ramblock.h? Is this ramblock change needed for this work?
On Tue, May 28, 2024 at 11:10:03AM -0400, Steven Sistare via wrote: > On 5/27/2024 2:20 PM, Peter Xu wrote: > > On Mon, Apr 29, 2024 at 08:55:16AM -0700, Steve Sistare wrote: > > > Define a type for the 256 byte id string to guarantee the same length is > > > used and enforced everywhere. > > > > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > > > --- > > > include/exec/ramblock.h | 3 ++- > > > include/migration/vmstate.h | 2 ++ > > > migration/savevm.c | 8 ++++---- > > > migration/vmstate.c | 3 ++- > > > 4 files changed, 10 insertions(+), 6 deletions(-) > > > > > > diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h > > > index 0babd10..61deefe 100644 > > > --- a/include/exec/ramblock.h > > > +++ b/include/exec/ramblock.h > > > @@ -23,6 +23,7 @@ > > > #include "cpu-common.h" > > > #include "qemu/rcu.h" > > > #include "exec/ramlist.h" > > > +#include "migration/vmstate.h" > > > struct RAMBlock { > > > struct rcu_head rcu; > > > @@ -35,7 +36,7 @@ struct RAMBlock { > > > void (*resized)(const char*, uint64_t length, void *host); > > > uint32_t flags; > > > /* Protected by the BQL. */ > > > - char idstr[256]; > > > + VMStateId idstr; > > > /* RCU-enabled, writes protected by the ramlist lock */ > > > QLIST_ENTRY(RAMBlock) next; > > > QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; > > > > Hmm.. Don't look like a good idea to include a migration header in > > ramblock.h? Is this ramblock change needed for this work? > > Well, entities that are migrated include migration headers, and now that > includes RAMBlock. There is precedent: > > 0 include/exec/ramblock.h 26 #include "migration/vmstate.h" > 1 include/hw/acpi/ich9_tco. 14 #include "migration/vmstate.h" > 2 include/hw/display/ramfb. 4 #include "migration/vmstate.h" > 3 include/hw/hyperv/vmbus.h 16 #include "migration/vmstate.h" > 4 include/hw/input/pl050.h 14 #include "migration/vmstate.h" > 5 include/hw/pci/shpc.h 7 #include "migration/vmstate.h" > 6 include/hw/virtio/virtio. 20 #include "migration/vmstate.h" > 7 include/migration/cpu.h 8 #include "migration/vmstate.h" > > Granted, only some of the C files that include ramblock.h need all of vmstate.h. > I could define VMStateId in a smaller file such as migration/misc.h, or a > new file migration/vmstateid.h, and include that in ramblock.h. > Any preference? One issue here is currently the idstr[] of ramblock is a verbose name of the ramblock, and logically it doesn't need to have anything to do with vmstate. I'll continue to read to see why we need VMStateID here for a ramblock. So if it is necessary, maybe the name VMStateID to be used here is misleading? It'll also be nice to separate the changes, as ramblock.h doesn't belong to migration subsystem but memory api. Thanks,
On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote: > How about a more general name for the type: > > migration/misc.h > typedef char (MigrationId)[256]; How about qemu/typedefs.h? Not sure whether it's applicable. Markus (in the loop) may have a better idea. Meanwhile, s/MigrationID/IDString/? > > exec/ramblock.h > struct RAMBlock { > MigrationId idstr; > > migration/savevm.c > typedef struct CompatEntry { > MigrationId idstr; > > typedef struct SaveStateEntry { > MigrationId idstr; > > > - Steve >
On Thu, May 30, 2024 at 01:11:26PM -0400, Steven Sistare wrote: > On 5/29/2024 2:53 PM, Peter Xu wrote: > > On Wed, May 29, 2024 at 01:30:18PM -0400, Steven Sistare wrote: > > > How about a more general name for the type: > > > > > > migration/misc.h > > > typedef char (MigrationId)[256]; > > > > How about qemu/typedefs.h? Not sure whether it's applicable. Markus (in > > the loop) may have a better idea. > > > > Meanwhile, s/MigrationID/IDString/? > > typedefs.h has a different purpose; giving short names to types > defined in internal include files. > > This id is specific to migration, so I still think its name should reflect > migration and it belongs in some include/migration/*.h file. > > ramblocks and migration are already closely related. There is nothing wrong > with including a migration header in ramblock.h so it can use a migration type. > We already have: > include/hw/acpi/ich9_tco.h:#include "migration/vmstate.h" > include/hw/display/ramfb.h:#include "migration/vmstate.h" > include/hw/input/pl050.h:#include "migration/vmstate.h" > include/hw/pci/shpc.h:#include "migration/vmstate.h" > include/hw/virtio/virtio.h:#include "migration/vmstate.h" > include/hw/hyperv/vmbus.h:#include "migration/vmstate.h" > > The 256 byte magic length already appears in too many places, and my code > would add more occurrences, so I really think that abstracting this type > would be cleaner. I agree having a typedef is nicer, but I don't understand why it must be migration related. It can be the type QEMU uses to represent any string based ID, and that's a generic concept to me. Migration can have a wrapper to process that type, but then migration will include the generic header in that case, it feels more natural that way?
diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h index 0babd10..61deefe 100644 --- a/include/exec/ramblock.h +++ b/include/exec/ramblock.h @@ -23,6 +23,7 @@ #include "cpu-common.h" #include "qemu/rcu.h" #include "exec/ramlist.h" +#include "migration/vmstate.h" struct RAMBlock { struct rcu_head rcu; @@ -35,7 +36,7 @@ struct RAMBlock { void (*resized)(const char*, uint64_t length, void *host); uint32_t flags; /* Protected by the BQL. */ - char idstr[256]; + VMStateId idstr; /* RCU-enabled, writes protected by the ramlist lock */ QLIST_ENTRY(RAMBlock) next; QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers; diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 4691334..a39c0e6 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -1210,6 +1210,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, bool vmstate_section_needed(const VMStateDescription *vmsd, void *opaque); +typedef char (VMStateId)[256]; + #define VMSTATE_INSTANCE_ID_ANY -1 /* Returns: 0 on success, -1 on failure */ diff --git a/migration/savevm.c b/migration/savevm.c index a30bcd9..9b1a335 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -197,13 +197,13 @@ const VMStateInfo vmstate_info_timer = { typedef struct CompatEntry { - char idstr[256]; + VMStateId idstr; int instance_id; } CompatEntry; typedef struct SaveStateEntry { QTAILQ_ENTRY(SaveStateEntry) entry; - char idstr[256]; + VMStateId idstr; uint32_t instance_id; int alias_id; int version_id; @@ -814,7 +814,7 @@ int register_savevm_live(const char *idstr, void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque) { SaveStateEntry *se, *new_se; - char id[256] = ""; + VMStateId id = ""; if (obj) { char *oid = vmstate_if_get_id(obj); @@ -2650,7 +2650,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type) uint32_t instance_id, version_id, section_id; int64_t start_ts, end_ts; SaveStateEntry *se; - char idstr[256]; + VMStateId idstr; int ret; /* Read section start */ diff --git a/migration/vmstate.c b/migration/vmstate.c index ef26f26..437f156 100644 --- a/migration/vmstate.c +++ b/migration/vmstate.c @@ -471,7 +471,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd, trace_vmstate_subsection_load(vmsd->name); while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) { - char idstr[256], *idstr_ret; + VMStateId idstr; + char *idstr_ret; int ret; uint8_t version_id, len, size; const VMStateDescription *sub_vmsd;
Define a type for the 256 byte id string to guarantee the same length is used and enforced everywhere. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- include/exec/ramblock.h | 3 ++- include/migration/vmstate.h | 2 ++ migration/savevm.c | 8 ++++---- migration/vmstate.c | 3 ++- 4 files changed, 10 insertions(+), 6 deletions(-)