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

On 10/10/24 07:06, 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/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. Unfortunately we'll still need the version bump to prevent older libvirt from restoring a mapped-ram saved image, but we can avoid adding the 'features' field if we use the existing 'compressed' field (renamed to 'format', see commit bd6d7ebf62 and related b0dc8a923d) to control mapped-ram. And if we use the existing field for mapped-ram, do we even need a version setting in qemu.conf? Martin seemed receptive to using *_image_format for mapped-ram. What are your opinions on that? Regards, Jim