[libvirt] [PATCH] fdstream: drop delete argument
Laine Stump
laine at laine.org
Tue Aug 2 19:18:49 UTC 2011
On 08/02/2011 01:31 PM, Eric Blake wrote:
> Revert 6a1f5f568f8. Now that libvirt_iohelper no longer has a
> race where it can open() a file after the parent process has
> unlink()d the same file, it makes more sense to make the callers
> both create and unlink, rather than the caller create and the
> stream unlink.
I wasn't paying attention to the messages/patches related to the race
condition you reference, but this (caller creates and unlinks)
definitely seems cleaner than the other way. Beyond that, the patch
seems to be correct. ACK.
> * src/fdstream.h (virFDStreamOpenFile, virFDStreamCreateFile):
> Callers are responsible for deletion.
> * src/fdstream.c (virFDStreamOpenFileInternal): Don't leak created
> file on failure.
> (virFDStreamOpenFile, virFDStreamCreateFile): Drop parameter.
> * src/lxc/lxc_driver.c (lxcDomainOpenConsole): Update callers.
> * src/qemu/qemu_driver.c (qemuDomainScreenshot)
> (qemuDomainOpenConsole): Likewise.
> * src/storage/storage_driver.c (storageVolumeDownload)
> (storageVolumeUpload): Likewise.
> * src/uml/uml_driver.c (umlDomainOpenConsole): Likewise.
> * src/vbox/vbox_tmpl.c (vboxDomainScreenshot): Likewise.
> * src/xen/xen_driver.c (xenUnifiedDomainOpenConsole): Likewise.
> ---
>
> This is the bigger patch, which I'm worried may be too invasive
> for 0.9.4 this late in the release cycle, but is cleaner overall.
>
> src/fdstream.c | 23 +++++++++--------------
> src/fdstream.h | 6 ++----
> src/lxc/lxc_driver.c | 2 +-
> src/qemu/qemu_driver.c | 11 ++++++-----
> src/storage/storage_driver.c | 4 ++--
> src/uml/uml_driver.c | 2 +-
> src/vbox/vbox_tmpl.c | 8 ++------
> src/xen/xen_driver.c | 2 +-
> 8 files changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/src/fdstream.c b/src/fdstream.c
> index 2b7812f..b60162c 100644
> --- a/src/fdstream.c
> +++ b/src/fdstream.c
> @@ -505,8 +505,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
> unsigned long long offset,
> unsigned long long length,
> int oflags,
> - int mode,
> - bool delete)
> + int mode)
> {
> int fd = -1;
> int fds[2] = { -1, -1 };
> @@ -514,8 +513,8 @@ virFDStreamOpenFileInternal(virStreamPtr st,
> virCommandPtr cmd = NULL;
> int errfd = -1;
>
> - VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o delete=%d",
> - st, path, oflags, offset, length, mode, delete);
> + VIR_DEBUG("st=%p path=%s oflags=%x offset=%llu length=%llu mode=%o",
> + st, path, oflags, offset, length, mode);
>
> if (oflags& O_CREAT)
> fd = open(path, oflags, mode);
> @@ -593,9 +592,6 @@ virFDStreamOpenFileInternal(virStreamPtr st,
> if (virFDStreamOpenInternal(st, fd, cmd, errfd, length)< 0)
> goto error;
>
> - if (delete)
> - unlink(path);
> -
> return 0;
>
> error:
> @@ -603,6 +599,8 @@ error:
> VIR_FORCE_CLOSE(fds[0]);
> VIR_FORCE_CLOSE(fds[1]);
> VIR_FORCE_CLOSE(fd);
> + if (oflags& O_CREAT)
> + unlink(path);
> return -1;
> }
>
> @@ -610,8 +608,7 @@ int virFDStreamOpenFile(virStreamPtr st,
> const char *path,
> unsigned long long offset,
> unsigned long long length,
> - int oflags,
> - bool delete)
> + int oflags)
> {
> if (oflags& O_CREAT) {
> streamsReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -621,7 +618,7 @@ int virFDStreamOpenFile(virStreamPtr st,
> }
> return virFDStreamOpenFileInternal(st, path,
> offset, length,
> - oflags, 0, delete);
> + oflags, 0);
> }
>
> int virFDStreamCreateFile(virStreamPtr st,
> @@ -629,11 +626,9 @@ int virFDStreamCreateFile(virStreamPtr st,
> unsigned long long offset,
> unsigned long long length,
> int oflags,
> - mode_t mode,
> - bool delete)
> + mode_t mode)
> {
> return virFDStreamOpenFileInternal(st, path,
> offset, length,
> - oflags | O_CREAT,
> - mode, delete);
> + oflags | O_CREAT, mode);
> }
> diff --git a/src/fdstream.h b/src/fdstream.h
> index 76f0cd0..f9edb23 100644
> --- a/src/fdstream.h
> +++ b/src/fdstream.h
> @@ -37,14 +37,12 @@ int virFDStreamOpenFile(virStreamPtr st,
> const char *path,
> unsigned long long offset,
> unsigned long long length,
> - int oflags,
> - bool delete);
> + int oflags);
> int virFDStreamCreateFile(virStreamPtr st,
> const char *path,
> unsigned long long offset,
> unsigned long long length,
> int oflags,
> - mode_t mode,
> - bool delete);
> + mode_t mode);
>
> #endif /* __VIR_FDSTREAM_H_ */
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 7d99d27..2d94309 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -2889,7 +2889,7 @@ lxcDomainOpenConsole(virDomainPtr dom,
> }
>
> if (virFDStreamOpenFile(st, chr->source.data.file.path,
> - 0, 0, O_RDWR, false)< 0)
> + 0, 0, O_RDWR)< 0)
> goto cleanup;
>
> ret = 0;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0297159..5c6d1b8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2862,6 +2862,7 @@ qemuDomainScreenshot(virDomainPtr dom,
> char *tmp = NULL;
> int tmp_fd = -1;
> char *ret = NULL;
> + bool unlink_tmp = false;
>
> virCheckFlags(0, NULL);
>
> @@ -2906,27 +2907,25 @@ qemuDomainScreenshot(virDomainPtr dom,
> virReportSystemError(errno, _("mkstemp(\"%s\") failed"), tmp);
> goto endjob;
> }
> + unlink_tmp = true;
>
> virSecurityManagerSetSavedStateLabel(qemu_driver->securityManager, vm, tmp);
>
> qemuDomainObjEnterMonitor(driver, vm);
> if (qemuMonitorScreendump(priv->mon, tmp)< 0) {
> qemuDomainObjExitMonitor(driver, vm);
> - unlink(tmp);
> goto endjob;
> }
> qemuDomainObjExitMonitor(driver, vm);
>
> if (VIR_CLOSE(tmp_fd)< 0) {
> virReportSystemError(errno, _("unable to close %s"), tmp);
> - unlink(tmp);
> goto endjob;
> }
>
> - if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true)< 0) {
> + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY)< 0) {
> qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
> _("unable to open stream"));
> - unlink(tmp);
> goto endjob;
> }
>
> @@ -2934,6 +2933,8 @@ qemuDomainScreenshot(virDomainPtr dom,
>
> endjob:
> VIR_FORCE_CLOSE(tmp_fd);
> + if (unlink_tmp)
> + unlink(tmp);
> VIR_FREE(tmp);
>
> if (qemuDomainObjEndJob(driver, vm) == 0)
> @@ -9259,7 +9260,7 @@ qemuDomainOpenConsole(virDomainPtr dom,
> }
>
> if (virFDStreamOpenFile(st, chr->source.data.file.path,
> - 0, 0, O_RDWR, false)< 0)
> + 0, 0, O_RDWR)< 0)
> goto cleanup;
>
> ret = 0;
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 9c353e3..6715790 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1614,7 +1614,7 @@ storageVolumeDownload(virStorageVolPtr obj,
> if (virFDStreamOpenFile(stream,
> vol->target.path,
> offset, length,
> - O_RDONLY, false)< 0)
> + O_RDONLY)< 0)
> goto out;
>
> ret = 0;
> @@ -1678,7 +1678,7 @@ storageVolumeUpload(virStorageVolPtr obj,
> if (virFDStreamOpenFile(stream,
> vol->target.path,
> offset, length,
> - O_WRONLY, false)< 0)
> + O_WRONLY)< 0)
> goto out;
>
> ret = 0;
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 6d04120..a9cb7ef 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -2295,7 +2295,7 @@ umlDomainOpenConsole(virDomainPtr dom,
> }
>
> if (virFDStreamOpenFile(st, chr->source.data.file.path,
> - 0, 0, O_RDWR, false)< 0)
> + 0, 0, O_RDWR)< 0)
> goto cleanup;
>
> ret = 0;
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 66a0fe9..5c71729 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -8713,7 +8713,6 @@ vboxDomainScreenshot(virDomainPtr dom,
> if (NS_FAILED(rc) || !width || !height) {
> vboxError(VIR_ERR_OPERATION_FAILED, "%s",
> _("unable to get screen resolution"));
> - unlink(tmp);
> goto endjob;
> }
>
> @@ -8724,7 +8723,6 @@ vboxDomainScreenshot(virDomainPtr dom,
> if (NS_FAILED(rc)) {
> vboxError(VIR_ERR_OPERATION_FAILED, "%s",
> _("failed to take screenshot"));
> - unlink(tmp);
> goto endjob;
> }
>
> @@ -8732,20 +8730,17 @@ vboxDomainScreenshot(virDomainPtr dom,
> screenDataSize)< 0) {
> virReportSystemError(errno, _("unable to write data "
> "to '%s'"), tmp);
> - unlink(tmp);
> goto endjob;
> }
>
> if (VIR_CLOSE(tmp_fd)< 0) {
> virReportSystemError(errno, _("unable to close %s"), tmp);
> - unlink(tmp);
> goto endjob;
> }
>
> - if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY, true)< 0) {
> + if (virFDStreamOpenFile(st, tmp, 0, 0, O_RDONLY)< 0) {
> vboxError(VIR_ERR_OPERATION_FAILED, "%s",
> _("unable to open stream"));
> - unlink(tmp);
> goto endjob;
> }
>
> @@ -8761,6 +8756,7 @@ endjob:
> }
>
> VIR_FORCE_CLOSE(tmp_fd);
> + unlink(tmp);
> VIR_FREE(tmp);
> VBOX_RELEASE(machine);
> vboxIIDUnalloc(&iid);
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 76506fb..2c66143 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -2164,7 +2164,7 @@ xenUnifiedDomainOpenConsole(virDomainPtr dom,
> }
>
> if (virFDStreamOpenFile(st, chr->source.data.file.path,
> - 0, 0, O_RDWR, false)< 0)
> + 0, 0, O_RDWR)< 0)
> goto cleanup;
>
> ret = 0;
More information about the libvir-list
mailing list