Re: [libvirt] [PATCH v3 2/4] qemu: Always call virFileWrapperFdClose()
On Wed, Feb 20, 2019 at 01:16:34PM +0100, Andrea Bolognani wrote: > On Tue, 2019-02-19 at 17:01 +, Daniel P. Berrangé wrote: > > On Tue, Feb 19, 2019 at 05:52:29PM +0100, Andrea Bolognani wrote: > [...] > > > @@ -3231,6 +3231,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, > > > > > > cleanup: > > > VIR_FORCE_CLOSE(fd); > > > +qemuFileWrapperFDClose(vm, wrapperFd); > > > > Don't we need to check & propagate the return status of this, > > otherwise callers would mistakenly think qemuDomainSaveMemory > > has succeeeded, despite qemuFileWrapperFDClose having raised > > an error. Likewise all the other places below. > > In cases where qemuFileWrapperFDClose() returning an error was not > considered an overall failure in the existing code, I have preserved > that behavior. FWIW, I consider that existing code to be buggy. It is akin to ignoring the return status of close(). Data you think you've written can in fact be lost. We usually only ignore close() return value if we're already in an error cleanup scenario. Any success codepaths should always honour the close() status. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu: Always call virFileWrapperFdClose()
On Tue, 2019-02-19 at 17:01 +, Daniel P. Berrangé wrote: > On Tue, Feb 19, 2019 at 05:52:29PM +0100, Andrea Bolognani wrote: [...] > > @@ -3231,6 +3231,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, > > > > cleanup: > > VIR_FORCE_CLOSE(fd); > > +qemuFileWrapperFDClose(vm, wrapperFd); > > Don't we need to check & propagate the return status of this, > otherwise callers would mistakenly think qemuDomainSaveMemory > has succeeeded, despite qemuFileWrapperFDClose having raised > an error. Likewise all the other places below. In cases where qemuFileWrapperFDClose() returning an error was not considered an overall failure in the existing code, I have preserved that behavior. Then again, we're ultimately going to call virReportError() from it instead of just logging a message with VIR_WARN(), so perhaps not returning an overall failure would be confusing... -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/4] qemu: Always call virFileWrapperFdClose()
On Tue, Feb 19, 2019 at 05:52:29PM +0100, Andrea Bolognani wrote:
> Right now we're reporting errors in virFileWrapperFdFree(),
> but that's hardly the appropriate place to do so, as free
> functions are supposed to do nothing more than release
> allocated resources.
>
> We want to move that code back into virFileWrapperFdClose(),
> but before we can do that we need to make sure the function
> is actually called every time we're done processing the
> wrapped file. The cleanup path is the obvious candidate.
>
> For two of the users we can just move the call, but for the
> other two we need to duplicate it instead in order not to
> alter the existing behavior.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/qemu/qemu_driver.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 5118f4ad42..30f69b339b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3231,6 +3231,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
>
> cleanup:
> VIR_FORCE_CLOSE(fd);
> +qemuFileWrapperFDClose(vm, wrapperFd);
Don't we need to check & propagate the return status of this,
otherwise callers would mistakenly think qemuDomainSaveMemory
has succeeeded, despite qemuFileWrapperFDClose having raised
an error. Likewise all the other places below.
> virFileWrapperFdFree(wrapperFd);
> virObjectUnref(cfg);
>
> @@ -3834,9 +3835,10 @@ doCoreDump(virQEMUDriverPtr driver,
>
> cleanup:
> VIR_FORCE_CLOSE(fd);
> +qemuFileWrapperFDClose(vm, wrapperFd);
> +virFileWrapperFdFree(wrapperFd);
> if (ret != 0)
> unlink(path);
> -virFileWrapperFdFree(wrapperFd);
> VIR_FREE(compressedpath);
> virObjectUnref(cfg);
> return ret;
> @@ -7043,17 +7045,16 @@ qemuDomainRestoreFlags(virConnectPtr conn,
>
> ret = qemuDomainSaveImageStartVM(conn, driver, vm, , data, path,
> false, QEMU_ASYNC_JOB_START);
> -if (virFileWrapperFdClose(wrapperFd) < 0)
> -VIR_WARN("Failed to close %s", path);
>
> qemuProcessEndJob(driver, vm);
>
> cleanup:
> virDomainDefFree(def);
> VIR_FORCE_CLOSE(fd);
> +virFileWrapperFdClose(wrapperFd);
> +virFileWrapperFdFree(wrapperFd);
> virQEMUSaveDataFree(data);
> VIR_FREE(xmlout);
> -virFileWrapperFdFree(wrapperFd);
> if (vm && ret < 0)
> qemuDomainRemoveInactiveJob(driver, vm);
> virDomainObjEndAPI();
> @@ -7318,14 +7319,13 @@ qemuDomainObjRestore(virConnectPtr conn,
>
> ret = qemuDomainSaveImageStartVM(conn, driver, vm, , data, path,
> start_paused, asyncJob);
> -if (virFileWrapperFdClose(wrapperFd) < 0)
> -VIR_WARN("Failed to close %s", path);
>
> cleanup:
> virQEMUSaveDataFree(data);
> VIR_FREE(xmlout);
> virDomainDefFree(def);
> VIR_FORCE_CLOSE(fd);
> +virFileWrapperFdClose(wrapperFd);
> virFileWrapperFdFree(wrapperFd);
> return ret;
> }
> --
> 2.20.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/4] qemu: Always call virFileWrapperFdClose()
Right now we're reporting errors in virFileWrapperFdFree(),
but that's hardly the appropriate place to do so, as free
functions are supposed to do nothing more than release
allocated resources.
We want to move that code back into virFileWrapperFdClose(),
but before we can do that we need to make sure the function
is actually called every time we're done processing the
wrapped file. The cleanup path is the obvious candidate.
For two of the users we can just move the call, but for the
other two we need to duplicate it instead in order not to
alter the existing behavior.
Signed-off-by: Andrea Bolognani
---
src/qemu/qemu_driver.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5118f4ad42..30f69b339b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3231,6 +3231,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
cleanup:
VIR_FORCE_CLOSE(fd);
+qemuFileWrapperFDClose(vm, wrapperFd);
virFileWrapperFdFree(wrapperFd);
virObjectUnref(cfg);
@@ -3834,9 +3835,10 @@ doCoreDump(virQEMUDriverPtr driver,
cleanup:
VIR_FORCE_CLOSE(fd);
+qemuFileWrapperFDClose(vm, wrapperFd);
+virFileWrapperFdFree(wrapperFd);
if (ret != 0)
unlink(path);
-virFileWrapperFdFree(wrapperFd);
VIR_FREE(compressedpath);
virObjectUnref(cfg);
return ret;
@@ -7043,17 +7045,16 @@ qemuDomainRestoreFlags(virConnectPtr conn,
ret = qemuDomainSaveImageStartVM(conn, driver, vm, , data, path,
false, QEMU_ASYNC_JOB_START);
-if (virFileWrapperFdClose(wrapperFd) < 0)
-VIR_WARN("Failed to close %s", path);
qemuProcessEndJob(driver, vm);
cleanup:
virDomainDefFree(def);
VIR_FORCE_CLOSE(fd);
+virFileWrapperFdClose(wrapperFd);
+virFileWrapperFdFree(wrapperFd);
virQEMUSaveDataFree(data);
VIR_FREE(xmlout);
-virFileWrapperFdFree(wrapperFd);
if (vm && ret < 0)
qemuDomainRemoveInactiveJob(driver, vm);
virDomainObjEndAPI();
@@ -7318,14 +7319,13 @@ qemuDomainObjRestore(virConnectPtr conn,
ret = qemuDomainSaveImageStartVM(conn, driver, vm, , data, path,
start_paused, asyncJob);
-if (virFileWrapperFdClose(wrapperFd) < 0)
-VIR_WARN("Failed to close %s", path);
cleanup:
virQEMUSaveDataFree(data);
VIR_FREE(xmlout);
virDomainDefFree(def);
VIR_FORCE_CLOSE(fd);
+virFileWrapperFdClose(wrapperFd);
virFileWrapperFdFree(wrapperFd);
return ret;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
