Re: [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/12/2017 11:57 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
> 
> After 290a00e41d we know how to deal with file mount points.
> However, when cleaning up the temporary location for preserved
> mount points we are still calling rmdir(). This won't fly for
> files. We need to call unlink(). Now, since we don't really care
> if the cleanup succeeded or not (it's the best effort anyway), we
> can call both rmdir() and unlink() without need for
> differentiation between files and directories.
> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  src/qemu/qemu_domain.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

But why call both?

My recollection on this is that unlink() was preferred over rmdir()
unless you cared about the target not existing. It's also 'kinder' if
some existing reference was left on the file.

I would prefer just the unlink for my:

Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>

but you can convince me for having both as well...

John

Feels like an eblake question, but he's away for a couple weeks, sigh.

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 36fa450e8..23b92606e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8311,8 +8311,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>  
>      ret = 0;
>   cleanup:
> -    for (i = 0; i < ndevMountsPath; i++)
> +    for (i = 0; i < ndevMountsPath; i++) {
> +        /* The path can be either a regular file or a dir. */
>          rmdir(devMountsSavePath[i]);
> +        unlink(devMountsSavePath[i]);
> +    }
>      virStringListFreeCount(devMountsPath, ndevMountsPath);
>      virStringListFreeCount(devMountsSavePath, ndevMountsPath);
>      return ret;
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux