Re: [libvirt] [PATCH 3/3] qemu: Split if condition in qemuDomainSnapshotUndoSingleDiskActive

Since compilers are trying to optimize code they are allowed to reorder evaluation of conditions in if statement (okay, not in all cases, but they can in this one). Therefore if we do: if (stat(file, &st) == 0 && unlink(file) < 0) after compiler chews this it may get feeling that swapping order is a good idea. However, we obviously don't want to call stat() on just unlink()-ed file.

--- src/qemu/qemu_driver.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ed290..037d45c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9998,9 +9998,14 @@ qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, VIR_WARN("Unable to restore security label on %s", disk->src); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) VIR_WARN("Unable to release lock on %s", disk->src); + + /* Deliberately do not join these two ifs. Compiler may mix up + * the order of evaluation so unlink() may proceed stat() + * which is not what we want */ if (need_unlink && stat(disk->src, &st) == 0 && - st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src) < 0) - VIR_WARN("Unable to remove just-created %s", disk->src); + st.st_size == 0 && S_ISREG(st.st_mode)) + if (unlink(disk->src) < 0) + VIR_WARN("Unable to remove just-created %s", disk->src); /* Update vm in place to match changes. */ VIR_FREE(disk->src);