Re: [libvirt] [RFC PATCH 3/3] libvirt: Implement two-tier driver loading

2014-01-20 Thread Peter Krempa
On 12/21/13 05:14, Adam Walters wrote:
 This implements a two-tier driver loading system into libvirt. The two 
 classes of drivers are Libvirt drivers and Hypervisor drivers. Hypervisor 
 drivers are fairly self-explanatory, they provide domain services. Libvirt 
 drivers are sort of the backend drivers for those, like the secret and 
 storage drivers. In the two-tier loading system here, the Libvirt drivers 
 are all loaded and auto-started. Once those have finished, the Hypervisor 
 drivers are loaded and auto-started. By doing things in this manner, we do 
 not have to hard-code a driver loading order or roll our own dynamic 
 dependency-based loading algorithm, while still gaining the benefits of a 
 more orderly driver loading approach, which should help minimize the 
 possibility of a race condition during startup. If another race condition is 
 found, the code can be extended to provide any number of extra tiers without 
 much trouble.

Again, as in 2/3, please break the long line into some paragraphs.

 
 Signed-off-by: Adam Walters a...@pandorasboxen.com
 ---
  src/libvirt.c | 57 +
  1 file changed, 49 insertions(+), 8 deletions(-)
 
 diff --git a/src/libvirt.c b/src/libvirt.c
 index 77f481e..9c00491 100644
 --- a/src/libvirt.c
 +++ b/src/libvirt.c
 @@ -837,31 +837,72 @@ int virStateInitialize(bool privileged,
 void *opaque)
  {
  size_t i;
 +virStateDriverPtr virLibvirtStateDriverTab[MAX_DRIVERS];
 +int virLibvirtStateDriverTabCount = 0;
 +virStateDriverPtr virHypervisorStateDriverTab[MAX_DRIVERS];
 +int virHypervisorStateDriverTabCount = 0;
  
  if (virInitialize()  0)
  return -1;
  
  for (i = 0; i  virStateDriverTabCount; i++) {
 -if (virStateDriverTab[i]-stateInitialize) {
 +switch (virStateDriverTab[i]-stateType) {
 +case VIR_DRV_STATE_DRV_LIBVIRT:
 +virLibvirtStateDriverTab[virLibvirtStateDriverTabCount++] =
 +virStateDriverTab[i];
 +break;
 +case VIR_DRV_STATE_DRV_HYPERVISOR:
 +virHypervisorStateDriverTab[virHypervisorStateDriverTabCount++] =
 +virStateDriverTab[i];
 +break;
 +}
 +}

Hmmm, this duplicates the loading code for each driver tier. As we don't
really need to copy the driver structure pointers into separate arrays
to load each driver tier I'd suggest the following multi-pass algorithm:

for (driverTier = 0; driverTier  driverTierLast; driverTier++) {
for (i = 0; i  virStateDriverTabCount i++) {
if (virStateDriverTab[i]-stateType != driverTier)
continue;

... ALL THE EXISTING LOADER CODE ...
}
}

This'd save a lot of the code duplication and still would keep the
ordering you are trying to introduce. I like the overal idea of this series.

Peter






signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [RFC PATCH 3/3] libvirt: Implement two-tier driver loading

2013-12-20 Thread Adam Walters
This implements a two-tier driver loading system into libvirt. The two classes 
of drivers are Libvirt drivers and Hypervisor drivers. Hypervisor drivers 
are fairly self-explanatory, they provide domain services. Libvirt drivers are 
sort of the backend drivers for those, like the secret and storage drivers. In 
the two-tier loading system here, the Libvirt drivers are all loaded and 
auto-started. Once those have finished, the Hypervisor drivers are loaded and 
auto-started. By doing things in this manner, we do not have to hard-code a 
driver loading order or roll our own dynamic dependency-based loading 
algorithm, while still gaining the benefits of a more orderly driver loading 
approach, which should help minimize the possibility of a race condition during 
startup. If another race condition is found, the code can be extended to 
provide any number of extra tiers without much trouble.

Signed-off-by: Adam Walters a...@pandorasboxen.com
---
 src/libvirt.c | 57 +
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 77f481e..9c00491 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -837,31 +837,72 @@ int virStateInitialize(bool privileged,
void *opaque)
 {
 size_t i;
+virStateDriverPtr virLibvirtStateDriverTab[MAX_DRIVERS];
+int virLibvirtStateDriverTabCount = 0;
+virStateDriverPtr virHypervisorStateDriverTab[MAX_DRIVERS];
+int virHypervisorStateDriverTabCount = 0;
 
 if (virInitialize()  0)
 return -1;
 
 for (i = 0; i  virStateDriverTabCount; i++) {
-if (virStateDriverTab[i]-stateInitialize) {
+switch (virStateDriverTab[i]-stateType) {
+case VIR_DRV_STATE_DRV_LIBVIRT:
+virLibvirtStateDriverTab[virLibvirtStateDriverTabCount++] =
+virStateDriverTab[i];
+break;
+case VIR_DRV_STATE_DRV_HYPERVISOR:
+virHypervisorStateDriverTab[virHypervisorStateDriverTabCount++] =
+virStateDriverTab[i];
+break;
+}
+}
+
+for (i = 0; i  virLibvirtStateDriverTabCount; i++) {
+if (virLibvirtStateDriverTab[i]-stateInitialize) {
 VIR_DEBUG(Running global init for %s state driver,
-  virStateDriverTab[i]-name);
-if (virStateDriverTab[i]-stateInitialize(privileged,
+  virLibvirtStateDriverTab[i]-name);
+if (virLibvirtStateDriverTab[i]-stateInitialize(privileged,
   callback,
   opaque)  0) {
 virErrorPtr err = virGetLastError();
 VIR_ERROR(_(Initialization of %s state driver failed: %s),
-  virStateDriverTab[i]-name,
+  virLibvirtStateDriverTab[i]-name,
   err  err-message ? err-message : _(Unknown 
problem));
 return -1;
 }
 }
 }
 
-for (i = 0; i  virStateDriverTabCount; i++) {
-if (virStateDriverTab[i]-stateAutoStart) {
+for (i = 0; i  virLibvirtStateDriverTabCount; i++) {
+if (virLibvirtStateDriverTab[i]-stateAutoStart) {
+VIR_DEBUG(Running global auto start for %s state driver,
+  virLibvirtStateDriverTab[i]-name);
+virLibvirtStateDriverTab[i]-stateAutoStart();
+}
+}
+
+for (i = 0; i  virHypervisorStateDriverTabCount; i++) {
+if (virHypervisorStateDriverTab[i]-stateInitialize) {
+VIR_DEBUG(Running global init for %s state driver,
+  virHypervisorStateDriverTab[i]-name);
+if (virHypervisorStateDriverTab[i]-stateInitialize(privileged,
+  callback,
+  opaque)  0) {
+virErrorPtr err = virGetLastError();
+VIR_ERROR(_(Initialization of %s state driver failed: %s),
+  virHypervisorStateDriverTab[i]-name,
+  err  err-message ? err-message : _(Unknown 
problem));
+return -1;
+}
+}
+}
+
+for (i = 0; i  virHypervisorStateDriverTabCount; i++) {
+if (virHypervisorStateDriverTab[i]-stateAutoStart) {
 VIR_DEBUG(Running global auto start for %s state driver,
-  virStateDriverTab[i]-name);
-virStateDriverTab[i]-stateAutoStart();
+  virHypervisorStateDriverTab[i]-name);
+virHypervisorStateDriverTab[i]-stateAutoStart();
 }
 }
 return 0;
-- 
1.8.5.2

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