When used with internal snapshots there is no header to be used and no
memory state to be decompressed.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_saveimage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 73115af42d..2538732901 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -573,7 +573,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
  * @fd: FD pointer of memory state file
  * @path: path to memory state file
  * @snapshot: snapshot to load when starting QEMU process or NULL
- * @header: header from memory state file
+ * @header: header from memory state file or NULL
  * @cookie: cookie from memory state file
  * @asyncJob: type of asynchronous job
  * @start_flags: flags to start QEMU process with
@@ -605,7 +605,7 @@ qemuSaveImageStartProcess(virConnectPtr conn,
     g_autofree char *errbuf = NULL;
     int rc = 0;
 
-    if ((header->version == 2) &&
+    if (header && (header->version == 2) &&
         (header->compressed != QEMU_SAVE_FORMAT_RAW)) {
         if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed)))
             return -1;
-- 
2.41.0

On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
> When used with internal snapshots there is no header to be used and no
> memory state to be decompressed.

This ...

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_saveimage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 73115af42d..2538732901 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -573,7 +573,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
>   * @fd: FD pointer of memory state file
>   * @path: path to memory state file
>   * @snapshot: snapshot to load when starting QEMU process or NULL
> - * @header: header from memory state file
> + * @header: header from memory state file or NULL
>   * @cookie: cookie from memory state file
>   * @asyncJob: type of asynchronous job
>   * @start_flags: flags to start QEMU process with

... here.

> @@ -605,7 +605,7 @@ qemuSaveImageStartProcess(virConnectPtr conn,
>      g_autofree char *errbuf = NULL;
>      int rc = 0;
>  
> -    if ((header->version == 2) &&
> +    if (header && (header->version == 2) &&
>          (header->compressed != QEMU_SAVE_FORMAT_RAW)) {
>          if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed)))
>              return -1;

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
> When used with internal snapshots there is no header to be used and no
> memory state to be decompressed.

I didn't yet have a look at the rest, but this made me curious. What are
you actually doing with this with internal snapshots?

There in fact isn't a save image at all with internal snapshots as the
memory image is stored in the qcow2 image (in a different section than
the data -> thus also the name internal) so I'm not exactly sure what
you are refering to here in the commit message.

On Thu, Aug 31, 2023 at 05:43:35PM +0200, Peter Krempa wrote:
> On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
> > When used with internal snapshots there is no header to be used and no
> > memory state to be decompressed.
> 
> I didn't yet have a look at the rest, but this made me curious. What are
> you actually doing with this with internal snapshots?
> 
> There in fact isn't a save image at all with internal snapshots as the
> memory image is stored in the qcow2 image (in a different section than
> the data -> thus also the name internal) so I'm not exactly sure what
> you are refering to here in the commit message.

All of that is correct. In PATCH 6/7 this new function is called from
qemuSnapshotRevertActive unconditionally. And it will handle reverting
internal and external snapshots and do the correct thing based on what
arguments are passed to it.

In case of internal snapshots we were calling qemuProcessStart and
passing virDomainMomentObj. To avoid code duplication this parameter
was introduced in PATCH 3/7 to this new helper so it can start QEMU
process when reverting to internal snapshot as well as when reverting to
external snapshots.

On Thu, Aug 31, 2023 at 17:59:27 +0200, Pavel Hrdina wrote:
> On Thu, Aug 31, 2023 at 05:43:35PM +0200, Peter Krempa wrote:
> > On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
> > > When used with internal snapshots there is no header to be used and no
> > > memory state to be decompressed.
> > 
> > I didn't yet have a look at the rest, but this made me curious. What are
> > you actually doing with this with internal snapshots?
> > 
> > There in fact isn't a save image at all with internal snapshots as the
> > memory image is stored in the qcow2 image (in a different section than
> > the data -> thus also the name internal) so I'm not exactly sure what
> > you are refering to here in the commit message.
> 
> All of that is correct. In PATCH 6/7 this new function is called from
> qemuSnapshotRevertActive unconditionally. And it will handle reverting
> internal and external snapshots and do the correct thing based on what
> arguments are passed to it.
> 
> In case of internal snapshots we were calling qemuProcessStart and
> passing virDomainMomentObj. To avoid code duplication this parameter
> was introduced in PATCH 3/7 to this new helper so it can start QEMU
> process when reverting to internal snapshot as well as when reverting to
> external snapshots.

Given how complex snapshots are I don't think it would make it any
cleaner if a single function is called that has two similar but
semantically very distinct behaviours and the reader has to understand
that it's based on the value of the arguments which behaviour is chosen.

Thus if you have both function calls and a condition selecting one of
them it will be easier for readers.