Re: [PATCH 07/20] qemu: QEMU_SAVE_VERSION: Bump to version 3

On Thu, Aug 08, 2024 at 05:38:00PM -0600, Jim Fehlig via Devel wrote: > QEMU's new mapped-ram stream format [1] is incompatible with the existing > sequential stream format. An older libvirt+QEMU that does not support > mapped-ram must not attempt to restore a mapped-ram saved image. Currently > the only way to achieve this is to bump QEMU_SAVE_VERSION. > > To avoid future version bumps, add a new 'features' element to the saved > image header. The element is used now to indicate the use of mapped-ram > feature, and provides a mechanism to support future save image features. > > [1] https://gitlab.com/qemu-project/qemu/-/blob/master/docs/devel/migration/m... > > Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; > --- > src/qemu/qemu_saveimage.c | 7 +++++++ > src/qemu/qemu_saveimage.h | 9 +++++++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c > index 018ab5a222..50fec33f54 100644 > --- a/src/qemu/qemu_saveimage.c > +++ b/src/qemu/qemu_saveimage.c > @@ -74,6 +74,7 @@ qemuSaveImageBswapHeader(virQEMUSaveHeader *hdr) > hdr->was_running = GUINT32_SWAP_LE_BE(hdr->was_running); > hdr->compressed = GUINT32_SWAP_LE_BE(hdr->compressed); > hdr->cookieOffset = GUINT32_SWAP_LE_BE(hdr->cookieOffset); > + hdr->features = GUINT32_SWAP_LE_BE(hdr->features); > } > > > @@ -637,6 +638,12 @@ qemuSaveImageOpen(virQEMUDriver *driver, > return -1; > }

> > + if (header->features && header->features != QEMU_SAVE_FEATURE_MAPPED_RAM) {

Can simplify: if ((header & ~(QEMU_SAVE_FEATURE_MAPPED_RAM)) != 0) and make the code patter future proof by ancitipating: if ((header & ~(QEMU_SAVE_FEATURE_MAPPED_RAM | QEMU_SAVE_FEATURE_BLAH)) != 0) > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("image contains unsupported features)")); > + return -1; > + } > + > if (header->cookieOffset) > xml_len = header->cookieOffset; > else > diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h > index e541792153..9dd7de292d 100644 > --- a/src/qemu/qemu_saveimage.h > +++ b/src/qemu/qemu_saveimage.h > @@ -28,10 +28,14 @@ > */ > #define QEMU_SAVE_MAGIC "LibvirtQemudSave" > #define QEMU_SAVE_PARTIAL "LibvirtQemudPart" > -#define QEMU_SAVE_VERSION 2 > +#define QEMU_SAVE_VERSION 3 This will make *all* save images incompatible with old libvirt, even if they're not using mapped ram. This feels sub-optimal to me. I figure we should use v2, unless the new header files are non-zero

> > G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL)); > > +typedef enum { > + QEMU_SAVE_FEATURE_MAPPED_RAM = 1 << 0, > +} qemuSaveFeatures; > + > typedef struct _virQEMUSaveHeader virQEMUSaveHeader; > struct _virQEMUSaveHeader { > char magic[sizeof(QEMU_SAVE_MAGIC)-1]; > @@ -40,7 +44,8 @@ struct _virQEMUSaveHeader { > uint32_t was_running; > uint32_t compressed; > uint32_t cookieOffset; > - uint32_t unused[14]; > + uint32_t features; Some features might be possible to restore from, even if the current impl doesn't know about it. In qcow2 they added "compatible features" and "incompatible featurs" fields to reflect that some are backwards compatible. So how about we do the same here with two 32-bit uints. > + uint32_t unused[13]; > }; > > > -- > 2.35.3 > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|