On Thu, Oct 10, 2024 at 02:06:47PM +0100, Daniel P. Berrangé wrote: > 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/mapped-ram.rst?ref_type=heads > > > > Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx> > > --- > > 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; > > } I'm not sure the code above this point is backwards compatible with v2. The code does 'saferead(.... sizeof(*header))', which will be over-reading data if the file was saved with v2..... > > > > + if (header->features && header->features != QEMU_SAVE_FEATURE_MAPPED_RAM) { ...and this check will be validating whatever random data followed the header in old libvirt save images. We must only validate 'features' for v3 images. > > 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 Perhaps just don't change this version until the next patch > > > > > 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 :| > 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 :|
