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@suse.com>
---
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) {
+ 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
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;
+ uint32_t unused[13];
};
--
2.35.3On 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@suse.com>
> ---
> 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 :|
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/mapped-ram.rst?ref_type=heads
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>> 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
Hi Daniel,
Have you had time to consider the idea of using the existing *_image_format
settings in qemu.conf to control mapped-ram? I'd like to know if that's a
possible approach before reworking this patch.
Regards,
Jim
On 10/10/24 15:03, Jim Fehlig wrote:
> 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/mapped-ram.rst?ref_type=heads
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>> ---
>>> 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
>
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@suse.com>
> > ---
> > 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 :|
© 2016 - 2024 Red Hat, Inc.