Move the code in qemuSaveImageCreate that opens, labels, and wraps the
save image fd to a helper function, providing more flexibility for
upcoming mapped-ram support.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
src/qemu/qemu_saveimage.c | 65 +++++++++++++++++++++++++++------------
1 file changed, 45 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 29b4e39879..e3f6a0ad0f 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -425,6 +425,50 @@ qemuSaveImageDecompressionStop(virCommand *cmd,
}
+static int
+qemuSaveImageCreateFd(virQEMUDriver *driver,
+ virDomainObj *vm,
+ const char *path,
+ virFileWrapperFd *wrapperFd,
+ bool *needUnlink,
+ unsigned int flags)
+{
+ g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+ int ret = -1;
+ VIR_AUTOCLOSE fd = -1;
+ int directFlag = 0;
+ unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
+
+ if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) {
+ wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
+ directFlag = virFileDirectFdFlag();
+ if (directFlag < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+ _("bypass cache unsupported by this system"));
+ return -1;
+ }
+ }
+
+ fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
+ O_WRONLY | O_TRUNC | O_CREAT | directFlag,
+ needUnlink);
+
+ if (fd < 0)
+ return -1;
+
+ if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
+ return -1;
+
+ if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
+ return -1;
+
+ ret = fd;
+ fd = -1;
+
+ return ret;
+}
+
+
/* Helper function to execute a migration to file with a correct save header
* the caller needs to make sure that the processors are stopped and do all other
* actions besides saving memory */
@@ -441,33 +485,14 @@ qemuSaveImageCreate(virQEMUDriver *driver,
bool needUnlink = false;
int ret = -1;
int fd = -1;
- int directFlag = 0;
virFileWrapperFd *wrapperFd = NULL;
- unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
/* Obtain the file handle. */
- if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) {
- wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
- directFlag = virFileDirectFdFlag();
- if (directFlag < 0) {
- virReportError(VIR_ERR_OPERATION_FAILED, "%s",
- _("bypass cache unsupported by this system"));
- goto cleanup;
- }
- }
+ fd = qemuSaveImageCreateFd(driver, vm, path, wrapperFd, &needUnlink, flags);
- fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
- O_WRONLY | O_TRUNC | O_CREAT | directFlag,
- &needUnlink);
if (fd < 0)
goto cleanup;
- if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
- goto cleanup;
-
- if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
- goto cleanup;
-
if (virQEMUSaveDataWrite(data, fd, path) < 0)
goto cleanup;
--
2.43.0On Wed, Mar 05, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote:
> Move the code in qemuSaveImageCreate that opens, labels, and wraps the
> save image fd to a helper function, providing more flexibility for
> upcoming mapped-ram support.
>
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> src/qemu/qemu_saveimage.c | 65 +++++++++++++++++++++++++++------------
> 1 file changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index 29b4e39879..e3f6a0ad0f 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -425,6 +425,50 @@ qemuSaveImageDecompressionStop(virCommand *cmd,
> }
>
>
> +static int
> +qemuSaveImageCreateFd(virQEMUDriver *driver,
> + virDomainObj *vm,
> + const char *path,
> + virFileWrapperFd *wrapperFd,
Doesn't this need to be virFileWrapperFd **, otherwise...
> + bool *needUnlink,
> + unsigned int flags)
> +{
> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> + int ret = -1;
> + VIR_AUTOCLOSE fd = -1;
> + int directFlag = 0;
> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
> +
> + if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) {
> + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
> + directFlag = virFileDirectFdFlag();
> + if (directFlag < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("bypass cache unsupported by this system"));
> + return -1;
> + }
> + }
> +
> + fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
> + O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> + needUnlink);
> +
> + if (fd < 0)
> + return -1;
> +
> + if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
> + return -1;
> +
> + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> + return -1;
....this assignment won't be visible to the caller
> +
> + ret = fd;
> + fd = -1;
> +
> + return ret;
> +}
> +
> +
> /* Helper function to execute a migration to file with a correct save header
> * the caller needs to make sure that the processors are stopped and do all other
> * actions besides saving memory */
> @@ -441,33 +485,14 @@ qemuSaveImageCreate(virQEMUDriver *driver,
> bool needUnlink = false;
> int ret = -1;
> int fd = -1;
> - int directFlag = 0;
> virFileWrapperFd *wrapperFd = NULL;
> - unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
...
> + fd = qemuSaveImageCreateFd(driver, vm, path, wrapperFd, &needUnlink, flags);
..This would neeed to be &wrapperFd
>
> - fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
> - O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> - &needUnlink);
> if (fd < 0)
> goto cleanup;
>
> - if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
> - goto cleanup;
> -
> - if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> - goto cleanup;
> -
> if (virQEMUSaveDataWrite(data, fd, path) < 0)
> goto cleanup;
>
> --
> 2.43.0
>
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 3/19/25 06:49, Daniel P. Berrangé wrote:
> On Wed, Mar 05, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote:
>> Move the code in qemuSaveImageCreate that opens, labels, and wraps the
>> save image fd to a helper function, providing more flexibility for
>> upcoming mapped-ram support.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>> src/qemu/qemu_saveimage.c | 65 +++++++++++++++++++++++++++------------
>> 1 file changed, 45 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
>> index 29b4e39879..e3f6a0ad0f 100644
>> --- a/src/qemu/qemu_saveimage.c
>> +++ b/src/qemu/qemu_saveimage.c
>> @@ -425,6 +425,50 @@ qemuSaveImageDecompressionStop(virCommand *cmd,
>> }
>>
>>
>> +static int
>> +qemuSaveImageCreateFd(virQEMUDriver *driver,
>> + virDomainObj *vm,
>> + const char *path,
>> + virFileWrapperFd *wrapperFd,
>
> Doesn't this need to be virFileWrapperFd **, otherwise...
>
>> + bool *needUnlink,
>> + unsigned int flags)
>> +{
>> + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>> + int ret = -1;
>> + VIR_AUTOCLOSE fd = -1;
>> + int directFlag = 0;
>> + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
>> +
>> + if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) {
>> + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
>> + directFlag = virFileDirectFdFlag();
>> + if (directFlag < 0) {
>> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> + _("bypass cache unsupported by this system"));
>> + return -1;
>> + }
>> + }
>> +
>> + fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
>> + O_WRONLY | O_TRUNC | O_CREAT | directFlag,
>> + needUnlink);
>> +
>> + if (fd < 0)
>> + return -1;
>> +
>> + if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
>> + return -1;
>> +
>> + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
>> + return -1;
>
> ....this assignment won't be visible to the caller
Opps, which makes qemuDomainFileWrapperFDClose a NOP in all cases. I've applied
the below diff locally. Any need to respin this series?
Regards,
Jim
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index e3f6a0ad0f..7ab44edcdc 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -429,7 +429,7 @@ static int
qemuSaveImageCreateFd(virQEMUDriver *driver,
virDomainObj *vm,
const char *path,
- virFileWrapperFd *wrapperFd,
+ virFileWrapperFd **wrapperFd,
bool *needUnlink,
unsigned int flags)
{
@@ -459,7 +459,7 @@ qemuSaveImageCreateFd(virQEMUDriver *driver,
if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
return -1;
- if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
+ if (!(*wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
return -1;
ret = fd;
@@ -488,7 +488,7 @@ qemuSaveImageCreate(virQEMUDriver *driver,
virFileWrapperFd *wrapperFd = NULL;
/* Obtain the file handle. */
- fd = qemuSaveImageCreateFd(driver, vm, path, wrapperFd, &needUnlink, flags);
+ fd = qemuSaveImageCreateFd(driver, vm, path, &wrapperFd, &needUnlink, flags);
if (fd < 0)
goto cleanup;
On Wed, Mar 19, 2025 at 05:09:01PM -0600, Jim Fehlig wrote:
> On 3/19/25 06:49, Daniel P. Berrangé wrote:
> > On Wed, Mar 05, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote:
> > > Move the code in qemuSaveImageCreate that opens, labels, and wraps the
> > > save image fd to a helper function, providing more flexibility for
> > > upcoming mapped-ram support.
> > >
> > > Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> > > ---
> > > src/qemu/qemu_saveimage.c | 65 +++++++++++++++++++++++++++------------
> > > 1 file changed, 45 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> > > index 29b4e39879..e3f6a0ad0f 100644
> > > --- a/src/qemu/qemu_saveimage.c
> > > +++ b/src/qemu/qemu_saveimage.c
> > > @@ -425,6 +425,50 @@ qemuSaveImageDecompressionStop(virCommand *cmd,
> > > }
> > > +static int
> > > +qemuSaveImageCreateFd(virQEMUDriver *driver,
> > > + virDomainObj *vm,
> > > + const char *path,
> > > + virFileWrapperFd *wrapperFd,
> >
> > Doesn't this need to be virFileWrapperFd **, otherwise...
> >
> > > + bool *needUnlink,
> > > + unsigned int flags)
> > > +{
> > > + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
> > > + int ret = -1;
> > > + VIR_AUTOCLOSE fd = -1;
> > > + int directFlag = 0;
> > > + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING;
> > > +
> > > + if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) {
> > > + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE;
> > > + directFlag = virFileDirectFdFlag();
> > > + if (directFlag < 0) {
> > > + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > > + _("bypass cache unsupported by this system"));
> > > + return -1;
> > > + }
> > > + }
> > > +
> > > + fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path,
> > > + O_WRONLY | O_TRUNC | O_CREAT | directFlag,
> > > + needUnlink);
> > > +
> > > + if (fd < 0)
> > > + return -1;
> > > +
> > > + if (qemuSecuritySetImageFDLabel(driver->securityManager, vm->def, fd) < 0)
> > > + return -1;
> > > +
> > > + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags)))
> > > + return -1;
> >
> > ....this assignment won't be visible to the caller
>
> Opps, which makes qemuDomainFileWrapperFDClose a NOP in all cases. I've
> applied the below diff locally. Any need to respin this series?
..snip..
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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 - 2026 Red Hat, Inc.