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@xxxxxxxxxx>
>> 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@xxxxxxxxxx>
>> Reviewed-by:Chuan Zheng <zhengchuan@xxxxxxxxxx>
>> Reviewed-by:alexchen <alex.chen@xxxxxxxxxx>
>> ---
>> 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
>
