Re: [libvirt PATCH] utils: add mutex to avoid races in virfile
Yes, it a good idea. I will fix it.
On 2020/6/16 23:16, Daniel P. Berrangé wrote:
On Tue, Jun 16, 2020 at 10:39:46PM +0800, Bihong Yu wrote:
> >From d9f7ed2af581222804392f9b93dc6aaf7d8c8995 Mon Sep 17 00:00:00 2001
> From: Bihong Yu <yubihong(a)huawei.com>
> Date: Tue, 16 Jun 2020 22:08:55 +0800
> Subject: [PATCH] utils: add mutex to avoid races in virfile
>
> There are races condiction to make '/run/libvirt/qemu/dbus' directory in
> virDirCreateNoFork() while concurrent start VMs, and get "failed to create
> directory '/run/libvirt/qemu/dbus': File exists" error message. Add an
> mutex to avoid races.
>
> Signed-off-by:Bihong Yu <yubihong(a)huawei.com>
> Reviewed-by:Chuan Zheng <zhengchuan(a)huawei.com>
> Reviewed-by:alexchen <alex.chen(a)huawei.com>
> ---
> src/util/virfile.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 20260a2..ae02a6e 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -88,6 +88,7 @@
> #include "virstring.h"
> #include "virutil.h"
> #include "virsocket.h"
> +#include "virthread.h"
>
> #define VIR_FROM_THIS VIR_FROM_NONE
>
> @@ -108,6 +109,9 @@ VIR_LOG_INIT("util.file");
> # define O_DIRECT 0
> #endif
>
> +/* Global mutex to avoid races */
> +virMutex fileLock = VIR_MUTEX_INITIALIZER;
> +
> int virFileClose(int *fdptr, virFileCloseFlags flags)
> {
> int saved_errno = 0;
> @@ -2612,15 +2616,18 @@ virDirCreateNoFork(const char *path,
> struct stat st;
> bool created = false;
>
> + virMutexLock(&fileLock);
> if (!((flags & VIR_DIR_CREATE_ALLOW_EXIST) && virFileExists(path)))
{
> if (mkdir(path, mode) < 0) {
Instead of adding a mutex we should get rid of the virFileExists check
entirely. Instead check "errno == EEXIST && (flags &
VIR_DIR_CREATE_ALLOW_EXIST)"
before reporting an error.
> ret = -errno;
> virReportSystemError(errno, _("failed to create directory
'%s'"),
> path);
> + virMutexUnlock(&fileLock);
> goto error;
> }
> created = true;
> }
> + virMutexUnlock(&fileLock);
>
> if (stat(path, &st) == -1) {
> ret = -errno;
Regards,
Daniel