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;
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;
--
1.8.3.1On 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?
--
Peter Xu
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?
- Steve
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,
--
Peter Xu
On 5/28/2024 1:44 PM, Peter Xu wrote:
> 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.
cpr requires migrating vmstate for ramblock. See the physmem patches for
why. vmstate requires a unique id, and ramblock already defines a unique
id which is used to identify the block and dirty bitmap in the migration
stream.
How about a more general name for the type:
migration/misc.h
typedef char (MigrationId)[256];
exec/ramblock.h
struct RAMBlock {
MigrationId idstr;
migration/savevm.c
typedef struct CompatEntry {
MigrationId idstr;
typedef struct SaveStateEntry {
MigrationId idstr;
- Steve
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
>
--
Peter Xu
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.
- Steve
>> 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? -- Peter Xu
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>
© 2016 - 2026 Red Hat, Inc.