[PATCH] Workaround to fix race condition around libvirt init
# HG changeset patch # User Sharad Mishra <snmishra@us.ibm.com> # Date 1314379933 25200 # Node ID a42b68361ed9cb4f7d6f15de5b58ccc68e88ef38 # Parent a346baf140d64177a9dc1066677c307ee6518236 Workaround to fix race condition around libvirt init. This patch fixes the race condition caused when mutiple threads try to start a VM. This patch also fixes the issue of incorrect mem allocation for VSSD property - emulator. Signed-off-by: Sharad Mishra <snmishra@us.ibm.com> diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -28,6 +28,7 @@ #include <stdbool.h> #include <stdarg.h> #include <unistd.h> +#include <pthread.h> #include <libvirt/libvirt.h> #include <libvirt/virterror.h> @@ -45,6 +46,9 @@ #include "misc_util.h" #include "cs_util.h" +static pthread_mutex_t libvirt_mutex = PTHREAD_MUTEX_INITIALIZER; +/* libvirt library not initialized */ +static int libvirt_initialized = 0; #define URI_ENV "HYPURI" @@ -114,11 +118,15 @@ CU_DEBUG("Connecting to libvirt with uri `%s'", uri); + pthread_mutex_lock(&libvirt_mutex); + if (is_read_only()) conn = virConnectOpenReadOnly(uri); else conn = virConnectOpen(uri); + pthread_mutex_unlock(&libvirt_mutex); + if (!conn) { CU_DEBUG("Unable to connect to `%s'", uri); return NULL; @@ -530,7 +538,19 @@ bool libvirt_cim_init(void) { - return virInitialize() == 0; + int ret=0; + + /* double-check lock pattern used for performance reasons */ + if (0 == libvirt_initialized) { + pthread_mutex_lock(&libvirt_mutex); + if (0 == libvirt_initialized) { + ret = virInitialize(); + if (ret == 0) + libvirt_initialized=1; + } + pthread_mutex_unlock(&libvirt_mutex); + } + return (ret == 0); } bool check_refs_pfx_match(const CMPIObjectPath *refa, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -188,6 +188,20 @@ return 1; } +static bool make_space(struct virt_device **list, int cur, int new) +{ + struct virt_device *tmp; + + tmp = calloc(cur + new, sizeof(*tmp)); + if (tmp == NULL) + return false; + + memcpy(tmp, *list, sizeof(*tmp) * cur); + *list = tmp; + + return true; +} + static bool fv_set_emulator(struct domain *domain, const char *emu) { @@ -198,6 +212,11 @@ if (emu == NULL) return true; + if (!make_space(&domain->dev_emu, 0, 1)) { + CU_DEBUG("Failed to alloc disk list"); + return false; + } + cleanup_virt_device(domain->dev_emu); domain->dev_emu->type = CIM_RES_TYPE_EMU; @@ -1369,20 +1388,6 @@ return msg; } -static bool make_space(struct virt_device **list, int cur, int new) -{ - struct virt_device *tmp; - - tmp = calloc(cur + new, sizeof(*tmp)); - if (tmp == NULL) - return false; - - memcpy(tmp, *list, sizeof(*tmp) * cur); - *list = tmp; - - return true; -} - static char *add_device_nodup(struct virt_device *dev, struct virt_device *list, int max,
On 08/26/2011 02:32 PM, Sharad Mishra wrote:
# HG changeset patch # User Sharad Mishra<snmishra@us.ibm.com> # Date 1314379933 25200 # Node ID a42b68361ed9cb4f7d6f15de5b58ccc68e88ef38 # Parent a346baf140d64177a9dc1066677c307ee6518236 Workaround to fix race condition around libvirt init.
This patch fixes the race condition caused when mutiple threads try to start a VM. This patch also fixes the issue of incorrect mem allocation for VSSD property - emulator.
Signed-off-by: Sharad Mishra<snmishra@us.ibm.com>
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -28,6 +28,7 @@ #include<stdbool.h> #include<stdarg.h> #include<unistd.h> +#include<pthread.h> #include<libvirt/libvirt.h> #include<libvirt/virterror.h>
@@ -45,6 +46,9 @@ #include "misc_util.h" #include "cs_util.h"
+static pthread_mutex_t libvirt_mutex = PTHREAD_MUTEX_INITIALIZER; +/* libvirt library not initialized */ +static int libvirt_initialized = 0;
#define URI_ENV "HYPURI"
@@ -114,11 +118,15 @@
CU_DEBUG("Connecting to libvirt with uri `%s'", uri);
+ pthread_mutex_lock(&libvirt_mutex); + if (is_read_only()) conn = virConnectOpenReadOnly(uri); else conn = virConnectOpen(uri);
+ pthread_mutex_unlock(&libvirt_mutex); + if (!conn) { CU_DEBUG("Unable to connect to `%s'", uri); return NULL; @@ -530,7 +538,19 @@
bool libvirt_cim_init(void) { - return virInitialize() == 0; + int ret=0; + + /* double-check lock pattern used for performance reasons */ + if (0 == libvirt_initialized) { + pthread_mutex_lock(&libvirt_mutex); + if (0 == libvirt_initialized) { + ret = virInitialize(); + if (ret == 0) + libvirt_initialized=1; + } + pthread_mutex_unlock(&libvirt_mutex); + } + return (ret == 0); }
Please check the coding style for this snippet.
bool check_refs_pfx_match(const CMPIObjectPath *refa, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -188,6 +188,20 @@ return 1; }
+static bool make_space(struct virt_device **list, int cur, int new) +{ + struct virt_device *tmp; + + tmp = calloc(cur + new, sizeof(*tmp)); + if (tmp == NULL) + return false; + + memcpy(tmp, *list, sizeof(*tmp) * cur); + *list = tmp; + + return true; +} +
This undoes the patch in revision 1134 "VirtualSystemManagementService: Fix yet another possible leak". In resume, NACK! Please review and post again. -- Eduardo de Barros Lima Software Engineer, Open Virtualization Linux Technology Center - IBM/Brazil eblima@br.ibm.com
I thought libvirt should guarantee that it would not start a VM twice, but if you fix that in CIM, I thought it is OK too. 于 2011-8-27 1:32, Sharad Mishra 写道:
# HG changeset patch # User Sharad Mishra<snmishra@us.ibm.com> # Date 1314379933 25200 # Node ID a42b68361ed9cb4f7d6f15de5b58ccc68e88ef38 # Parent a346baf140d64177a9dc1066677c307ee6518236 Workaround to fix race condition around libvirt init.
This patch fixes the race condition caused when mutiple threads try to start a VM. This patch also fixes the issue of incorrect mem allocation for VSSD property - emulator.
Signed-off-by: Sharad Mishra<snmishra@us.ibm.com>
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -28,6 +28,7 @@ #include<stdbool.h> #include<stdarg.h> #include<unistd.h> +#include<pthread.h> #include<libvirt/libvirt.h> #include<libvirt/virterror.h>
@@ -45,6 +46,9 @@ #include "misc_util.h" #include "cs_util.h"
+static pthread_mutex_t libvirt_mutex = PTHREAD_MUTEX_INITIALIZER; +/* libvirt library not initialized */ +static int libvirt_initialized = 0;
#define URI_ENV "HYPURI"
@@ -114,11 +118,15 @@
CU_DEBUG("Connecting to libvirt with uri `%s'", uri);
+ pthread_mutex_lock(&libvirt_mutex); + if (is_read_only()) conn = virConnectOpenReadOnly(uri); else conn = virConnectOpen(uri);
+ pthread_mutex_unlock(&libvirt_mutex); + if (!conn) { CU_DEBUG("Unable to connect to `%s'", uri); return NULL; @@ -530,7 +538,19 @@
bool libvirt_cim_init(void) { - return virInitialize() == 0; + int ret=0; + + /* double-check lock pattern used for performance reasons */ + if (0 == libvirt_initialized) { + pthread_mutex_lock(&libvirt_mutex); + if (0 == libvirt_initialized) { + ret = virInitialize(); + if (ret == 0) + libvirt_initialized=1; + } + pthread_mutex_unlock(&libvirt_mutex); + } + return (ret == 0); }
bool check_refs_pfx_match(const CMPIObjectPath *refa, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -188,6 +188,20 @@ return 1; }
+static bool make_space(struct virt_device **list, int cur, int new) +{ + struct virt_device *tmp; + + tmp = calloc(cur + new, sizeof(*tmp)); + if (tmp == NULL) + return false; + + memcpy(tmp, *list, sizeof(*tmp) * cur); + *list = tmp; + + return true; +} + static bool fv_set_emulator(struct domain *domain, const char *emu) { @@ -198,6 +212,11 @@ if (emu == NULL) return true;
+ if (!make_space(&domain->dev_emu, 0, 1)) { + CU_DEBUG("Failed to alloc disk list"); + return false; + } + cleanup_virt_device(domain->dev_emu);
domain->dev_emu->type = CIM_RES_TYPE_EMU; @@ -1369,20 +1388,6 @@ return msg; }
-static bool make_space(struct virt_device **list, int cur, int new) -{ - struct virt_device *tmp; - - tmp = calloc(cur + new, sizeof(*tmp)); - if (tmp == NULL) - return false; - - memcpy(tmp, *list, sizeof(*tmp) * cur); - *list = tmp; - - return true; -} - static char *add_device_nodup(struct virt_device *dev, struct virt_device *list, int max,
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803
libvirt-cim-bounces@redhat.com wrote on 08/28/2011 09:06:49 PM:
Wayne Xia <xiawenc@linux.vnet.ibm.com> Sent by: libvirt-cim-bounces@redhat.com
08/28/11 09:06 PM
Please respond to List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>
To
List for discussion and development of libvirt CIM <libvirt-cim@redhat.com>
cc
Subject
Re: [Libvirt-cim] [PATCH] Workaround to fix race condition around libvirt init
I thought libvirt should guarantee that it would not start a VM twice, but if you fix that in CIM, I thought it is OK too.
My comments below are misleading. I meant to say multiple threads are trying to start multiple VMs at the same time and each thread is starting a unique VM. So the situation is ... you have 6 VMs to start on the host and you start 6 threads to start those VMs. I will correct the comment in next rev of this patch.
于 2011-8-27 1:32, Sharad Mishra 写道:
# HG changeset patch # User Sharad Mishra<snmishra@us.ibm.com> # Date 1314379933 25200 # Node ID a42b68361ed9cb4f7d6f15de5b58ccc68e88ef38 # Parent a346baf140d64177a9dc1066677c307ee6518236 Workaround to fix race condition around libvirt init.
This patch fixes the race condition caused when mutiple threads try to start a VM. This patch also fixes the issue of incorrect mem allocation for VSSD property - emulator.
Signed-off-by: Sharad Mishra<snmishra@us.ibm.com>
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -28,6 +28,7 @@ #include<stdbool.h> #include<stdarg.h> #include<unistd.h> +#include<pthread.h> #include<libvirt/libvirt.h> #include<libvirt/virterror.h>
@@ -45,6 +46,9 @@ #include "misc_util.h" #include "cs_util.h"
+static pthread_mutex_t libvirt_mutex = PTHREAD_MUTEX_INITIALIZER; +/* libvirt library not initialized */ +static int libvirt_initialized = 0;
#define URI_ENV "HYPURI"
@@ -114,11 +118,15 @@
CU_DEBUG("Connecting to libvirt with uri `%s'", uri);
+ pthread_mutex_lock(&libvirt_mutex); + if (is_read_only()) conn = virConnectOpenReadOnly(uri); else conn = virConnectOpen(uri);
+ pthread_mutex_unlock(&libvirt_mutex); + if (!conn) { CU_DEBUG("Unable to connect to `%s'", uri); return NULL; @@ -530,7 +538,19 @@
bool libvirt_cim_init(void) { - return virInitialize() == 0; + int ret=0; + + /* double-check lock pattern used for performance reasons */ + if (0 == libvirt_initialized) { + pthread_mutex_lock(&libvirt_mutex); + if (0 == libvirt_initialized) { + ret = virInitialize(); + if (ret == 0) + libvirt_initialized=1; + } + pthread_mutex_unlock(&libvirt_mutex); + } + return (ret == 0); }
bool check_refs_pfx_match(const CMPIObjectPath *refa, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/ Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -188,6 +188,20 @@ return 1; }
+static bool make_space(struct virt_device **list, int cur, int new) +{ + struct virt_device *tmp; + + tmp = calloc(cur + new, sizeof(*tmp)); + if (tmp == NULL) + return false; + + memcpy(tmp, *list, sizeof(*tmp) * cur); + *list = tmp; + + return true; +} + static bool fv_set_emulator(struct domain *domain, const char *emu) { @@ -198,6 +212,11 @@ if (emu == NULL) return true;
+ if (!make_space(&domain->dev_emu, 0, 1)) { + CU_DEBUG("Failed to alloc disk list"); + return false; + } + cleanup_virt_device(domain->dev_emu);
domain->dev_emu->type = CIM_RES_TYPE_EMU; @@ -1369,20 +1388,6 @@ return msg; }
-static bool make_space(struct virt_device **list, int cur, int new) -{ - struct virt_device *tmp; - - tmp = calloc(cur + new, sizeof(*tmp)); - if (tmp == NULL) - return false; - - memcpy(tmp, *list, sizeof(*tmp) * cur); - *list = tmp; - - return true; -} - static char *add_device_nodup(struct virt_device *dev, struct virt_device *list, int max,
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards
Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
Is it possible that libvirt would invoke one VM twice? also some comments below. 于 2011-8-27 1:32, Sharad Mishra 写道:
# HG changeset patch # User Sharad Mishra<snmishra@us.ibm.com> # Date 1314379933 25200 # Node ID a42b68361ed9cb4f7d6f15de5b58ccc68e88ef38 # Parent a346baf140d64177a9dc1066677c307ee6518236 Workaround to fix race condition around libvirt init.
This patch fixes the race condition caused when mutiple threads try to start a VM. This patch also fixes the issue of incorrect mem allocation for VSSD property - emulator.
Signed-off-by: Sharad Mishra<snmishra@us.ibm.com>
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -28,6 +28,7 @@ #include<stdbool.h> #include<stdarg.h> #include<unistd.h> +#include<pthread.h> #include<libvirt/libvirt.h> #include<libvirt/virterror.h>
@@ -45,6 +46,9 @@ #include "misc_util.h" #include "cs_util.h"
+static pthread_mutex_t libvirt_mutex = PTHREAD_MUTEX_INITIALIZER; +/* libvirt library not initialized */ +static int libvirt_initialized = 0;
#define URI_ENV "HYPURI"
@@ -114,11 +118,15 @@
CU_DEBUG("Connecting to libvirt with uri `%s'", uri);
+ pthread_mutex_lock(&libvirt_mutex); + if (is_read_only()) conn = virConnectOpenReadOnly(uri); else conn = virConnectOpen(uri);
+ pthread_mutex_unlock(&libvirt_mutex); + if (!conn) { CU_DEBUG("Unable to connect to `%s'", uri); return NULL; @@ -530,7 +538,19 @@
bool libvirt_cim_init(void) { - return virInitialize() == 0; + int ret=0; + + /* double-check lock pattern used for performance reasons */ + if (0 == libvirt_initialized) { + pthread_mutex_lock(&libvirt_mutex); + if (0 == libvirt_initialized) { + ret = virInitialize(); + if (ret == 0) + libvirt_initialized=1; + } + pthread_mutex_unlock(&libvirt_mutex); + } + return (ret == 0); }
bool check_refs_pfx_match(const CMPIObjectPath *refa, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -188,6 +188,20 @@ return 1; }
+static bool make_space(struct virt_device **list, int cur, int new) +{ + struct virt_device *tmp; + + tmp = calloc(cur + new, sizeof(*tmp)); + if (tmp == NULL) + return false; + maybe some lines should be added here like this:
if (*list!=NULL) { memcpy(tmp, *list, sizeof(*tmp) * cur); free(*list); *list = tmp; }
+ memcpy(tmp, *list, sizeof(*tmp) * cur); + *list = tmp; + + return true; +} + static bool fv_set_emulator(struct domain *domain, const char *emu) { @@ -198,6 +212,11 @@ if (emu == NULL) return true;
+ if (!make_space(&domain->dev_emu, 0, 1)) { + CU_DEBUG("Failed to alloc disk list"); + return false; + } + cleanup_virt_device(domain->dev_emu);
domain->dev_emu->type = CIM_RES_TYPE_EMU; @@ -1369,20 +1388,6 @@ return msg; }
-static bool make_space(struct virt_device **list, int cur, int new) -{ - struct virt_device *tmp; - - tmp = calloc(cur + new, sizeof(*tmp)); - if (tmp == NULL) - return false; - - memcpy(tmp, *list, sizeof(*tmp) * cur); - *list = tmp; - - return true; -} - static char *add_device_nodup(struct virt_device *dev, struct virt_device *list, int max,
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803
Wayne Xia <xiawenc@linux.vnet.ibm.com> wrote on 08/29/2011 02:47:34 AM:
Wayne Xia <xiawenc@linux.vnet.ibm.com> 08/29/11 02:47 AM
To
libvirt-cim@redhat.com, Sharad Mishra/Beaverton/IBM@IBMUS
cc
Subject
Re: [Libvirt-cim] [PATCH] Workaround to fix race condition around libvirt init
Thanks for the comment, Wayne.
Is it possible that libvirt would invoke one VM twice?
that will be a bug. I have never seen that happen.
also some comments below.
于 2011-8-27 1:32, Sharad Mishra 写道:
# HG changeset patch # User Sharad Mishra<snmishra@us.ibm.com> # Date 1314379933 25200 # Node ID a42b68361ed9cb4f7d6f15de5b58ccc68e88ef38 # Parent a346baf140d64177a9dc1066677c307ee6518236 Workaround to fix race condition around libvirt init.
This patch fixes the race condition caused when mutiple threads try to start a VM. This patch also fixes the issue of incorrect mem allocation for VSSD property - emulator.
Signed-off-by: Sharad Mishra<snmishra@us.ibm.com>
diff --git a/libxkutil/misc_util.c b/libxkutil/misc_util.c --- a/libxkutil/misc_util.c +++ b/libxkutil/misc_util.c @@ -28,6 +28,7 @@ #include<stdbool.h> #include<stdarg.h> #include<unistd.h> +#include<pthread.h> #include<libvirt/libvirt.h> #include<libvirt/virterror.h>
@@ -45,6 +46,9 @@ #include "misc_util.h" #include "cs_util.h"
+static pthread_mutex_t libvirt_mutex = PTHREAD_MUTEX_INITIALIZER; +/* libvirt library not initialized */ +static int libvirt_initialized = 0;
#define URI_ENV "HYPURI"
@@ -114,11 +118,15 @@
CU_DEBUG("Connecting to libvirt with uri `%s'", uri);
+ pthread_mutex_lock(&libvirt_mutex); + if (is_read_only()) conn = virConnectOpenReadOnly(uri); else conn = virConnectOpen(uri);
+ pthread_mutex_unlock(&libvirt_mutex); + if (!conn) { CU_DEBUG("Unable to connect to `%s'", uri); return NULL; @@ -530,7 +538,19 @@
bool libvirt_cim_init(void) { - return virInitialize() == 0; + int ret=0; + + /* double-check lock pattern used for performance reasons */ + if (0 == libvirt_initialized) { + pthread_mutex_lock(&libvirt_mutex); + if (0 == libvirt_initialized) { + ret = virInitialize(); + if (ret == 0) + libvirt_initialized=1; + } + pthread_mutex_unlock(&libvirt_mutex); + } + return (ret == 0); }
bool check_refs_pfx_match(const CMPIObjectPath *refa, diff --git a/src/Virt_VirtualSystemManagementService.c b/src/ Virt_VirtualSystemManagementService.c --- a/src/Virt_VirtualSystemManagementService.c +++ b/src/Virt_VirtualSystemManagementService.c @@ -188,6 +188,20 @@ return 1; }
+static bool make_space(struct virt_device **list, int cur, int new) +{ + struct virt_device *tmp; + + tmp = calloc(cur + new, sizeof(*tmp)); + if (tmp == NULL) + return false; + maybe some lines should be added here like this:
if (*list!=NULL) { memcpy(tmp, *list, sizeof(*tmp) * cur); free(*list); *list = tmp; }
Eduardo already submitted a patch for this. I will redo this patch to include Eduardo's fix.
+ memcpy(tmp, *list, sizeof(*tmp) * cur); + *list = tmp; + + return true; +} + static bool fv_set_emulator(struct domain *domain, const char *emu) { @@ -198,6 +212,11 @@ if (emu == NULL) return true;
+ if (!make_space(&domain->dev_emu, 0, 1)) { + CU_DEBUG("Failed to alloc disk list"); + return false; + } + cleanup_virt_device(domain->dev_emu);
domain->dev_emu->type = CIM_RES_TYPE_EMU; @@ -1369,20 +1388,6 @@ return msg; }
-static bool make_space(struct virt_device **list, int cur, int new) -{ - struct virt_device *tmp; - - tmp = calloc(cur + new, sizeof(*tmp)); - if (tmp == NULL) - return false; - - memcpy(tmp, *list, sizeof(*tmp) * cur); - *list = tmp; - - return true; -} - static char *add_device_nodup(struct virt_device *dev, struct virt_device *list, int max,
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim
-- Best Regards
Wayne Xia mail:xiawenc@linux.vnet.ibm.com tel:86-010-82450803
participants (3)
-
Eduardo Lima (Etrunko) -
Sharad Mishra -
Wayne Xia