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 <phrd...@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 :|
