The commit in question made an incorrect change that resulted in getting
O_RDONLY FD instead of O_RDWR preventing any writes to happen with the
following error:

virQEMUSaveDataWrite:176 : failed to write header to domain save file '/path/to/save.img': Bad file descriptor

Fixes: 517248e2394476a3105ff5866b0b718fc6583073
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 78bfaa5b3a..a35abf2747 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5949,7 +5949,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
     if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0)
         goto cleanup;
 
-    fd = qemuSaveImageOpen(driver, path, 0, NULL, false);
+    fd = qemuSaveImageOpen(driver, path, false, NULL, true);
     if (fd < 0)
         goto cleanup;
 
-- 
2.48.1

On Wed, Feb 12, 2025 at 11:21:25AM +0100, Pavel Hrdina wrote:
> The commit in question made an incorrect change that resulted in getting
> O_RDONLY FD instead of O_RDWR preventing any writes to happen with the
> following error:
> 
> virQEMUSaveDataWrite:176 : failed to write header to domain save file '/path/to/save.img': Bad file descriptor
> 
> Fixes: 517248e2394476a3105ff5866b0b718fc6583073
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 78bfaa5b3a..a35abf2747 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5949,7 +5949,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
>      if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0)
>          goto cleanup;
>  
> -    fd = qemuSaveImageOpen(driver, path, 0, NULL, false);
> +    fd = qemuSaveImageOpen(driver, path, false, NULL, true);
>      if (fd < 0)
>          goto cleanup;

Not an objection to your patch, just an observation about API design
in general. Parameters which are 'bool' flags actively encourage
this kind of mistake, even more so when there are multiple bools per
API. This is why of the reasons that glib has a general design practice
of defining enums for any flags.

eg

  typedef enum {
    QEMU_SAVE_IMAGE_OPEN_BYPASS_CACHE = (1 << 0),
    QEMU_SAVE_IMAGE_OPEN_WRITABLE = (1 << 1),
  } qemuSaveImageFlags;

  qemuSaveImageOpen(virQEMUDriver *driver,
                    const char *path,
                    enumqemuSaveImageFlags falgs,
                    virFileWrapperFd **wrapperFd);


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 Wed, Feb 12, 2025 at 11:21:25 +0100, Pavel Hrdina wrote:
> The commit in question made an incorrect change that resulted in getting
> O_RDONLY FD instead of O_RDWR preventing any writes to happen with the
> following error:
> 
> virQEMUSaveDataWrite:176 : failed to write header to domain save file '/path/to/save.img': Bad file descriptor
> 
> Fixes: 517248e2394476a3105ff5866b0b718fc6583073
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

Possibly mention the change to pass 'bypass_cache' as proper bool in the
commit message as well.

>  src/qemu/qemu_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 78bfaa5b3a..a35abf2747 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5949,7 +5949,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path,
>      if (qemuSaveImageGetMetadata(driver, NULL, path, &def, &data) < 0)
>          goto cleanup;
>  
> -    fd = qemuSaveImageOpen(driver, path, 0, NULL, false);
> +    fd = qemuSaveImageOpen(driver, path, false, NULL, true);
>      if (fd < 0)
>          goto cleanup;


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