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 <adam@xxxxxxxxxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list