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 <[email protected]>
> > ---
> > 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 :|
