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

[Daniel P. Berrange]

On Fri, Apr 13, 2012 at 11:12:54AM +0200, Michal Privoznik wrote:
> 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.

Really ? I'm not sure I believe that. IIRC in-order short-circuit
evaluation is a part of the C standard. Compilers can't do any
optimization which changes the order of evalation without breaking
countless C programs.

> ---
>  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);


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|