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.sist...@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
