Re: [libvirt] [PATCH 2/5] Convert virDomainObjListPtr to use a hash of domain objects

The current virDomainObjListPtr object stores domain objects in an array. This means that to find a particular objects requires O(n) time, and more critically acquiring O(n) mutex locks. The new impl replaces the array with a virHashTable, keyed off UUID. Finding a object based on UUID is now O(1) time, and only requires a single mutex lock. Finding by name/id is unchanged in complexity. In changing this, all code which iterates over the array had to be updated to use a hash table iterator function callback. Several of the functions which were identically duplicating across all drivers were pulled into domain_conf.c * src/conf/domain_conf.h, src/conf/domain_conf.c: Change  virDomainObjListPtr to use virHashTable. Add a initializer  method virDomainObjListInit, and rename virDomainObjListFree  to virDomainObjListDeinit, since its not actually freeing  the container, only its contents. Also add some convenient  methods virDomainObjListGetInactiveNames,  virDomainObjListGetActiveIDs and virDomainObjListNumOfDomains  which can be used to implement the correspondingly named  public API entry points in drivers * src/libvirt_private.syms: Export new methods from domain_conf.h * src/lxc/lxc_driver.c, src/opennebula/one_driver.c,  src/openvz/openvz_conf.c, src/openvz/openvz_driver.c,  src/qemu/qemu_driver.c, src/test/test_driver.c,  src/uml/uml_driver.c, src/vbox/vbox_tmpl.c: Update all code  to deal with hash tables instead of arrays for domains ---

+ +static void virDomainObjListCopyInactiveNames(void *payload, const char *name ATTRIBUTE_UNUSED, void *opaque) +{ +    virDomainObjPtr obj = payload; +    struct virDomainNameData *data = opaque;

+    virDomainObjLock(obj); +    if (!virDomainIsActive(obj) && data->numnames < data->maxnames) { +        if (!(data->names[data->numnames] = strdup(obj->def->name))) +            data->oom = 1; +        else +            data->numnames++; +    } +    virDomainObjUnlock(obj); +} + + +int virDomainObjListGetInactiveNames(virDomainObjListPtr doms, +                                     char **const names, +                                     int maxnames) +{ +    struct virDomainNameData data = { 0, 0, maxnames, names }; +    int i; +    virHashForEach(doms->objs, virDomainObjListCopyInactiveNames, &data); +    if (data.oom)

+        goto cleanup; + +    return data.numnames; + +cleanup: +    for (i = 0 ; i < data.numnames ; i++) +        VIR_FREE(data.names[i]); +    return -1; +} +

@@ -887,27 +856,15 @@ static void lxcMonitorEvent(int watch,                             int events ATTRIBUTE_UNUSED,                             void *data)  { -    lxc_driver_t *driver = data; -    virDomainObjPtr vm = NULL; +    lxc_driver_t *driver = lxc_driver; +    virDomainObjPtr vm = data;     virDomainEventPtr event = NULL; -    unsigned int i;     lxcDriverLock(driver); -    for (i = 0 ; i < driver->domains.count ; i++) { -        virDomainObjPtr tmpvm = driver->domains.objs[i]; -        virDomainObjLock(tmpvm); -        if (tmpvm->monitorWatch == watch) { -            vm = tmpvm; -            break; -        } -        virDomainObjUnlock(tmpvm); -    } -    if (!vm) { -        virEventRemoveHandle(watch); -        goto cleanup; -    } +    virDomainObjLock(vm); +    lxcDriverUnlock(driver); -    if (vm->monitor != fd) { +    if (vm->monitor != fd || vm->monitorWatch != watch) {         virEventRemoveHandle(watch);         goto cleanup;     } @@ -925,11 +882,9 @@ static void lxcMonitorEvent(int watch,     }  cleanup: -    if (vm) -        virDomainObjUnlock(vm); +    virDomainObjUnlock(vm);     if (event)         lxcDomainEventQueue(driver, event); -    lxcDriverUnlock(driver);  }

 static void  qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) { -    struct qemud_driver *driver = opaque; -    virDomainObjPtr vm = NULL; +    struct qemud_driver *driver = qemu_driver; +    virDomainObjPtr vm = opaque;     virDomainEventPtr event = NULL; -    unsigned int i;     int quit = 0, failed = 0; +    /* XXX Normally we have to lock the driver first, to protect +     * against someone adding/removing the domain. We know, +     * however, then if we're getting data in this callback +     * the VM must be running. Nowhere is allowed to remove +     * a domain while it is running, so it is safe to not +     * lock the driver here... */     qemuDriverLock(driver); -    for (i = 0 ; i < driver->domains.count ; i++) { -        virDomainObjPtr tmpvm = driver->domains.objs[i]; -        virDomainObjLock(tmpvm); -        if (virDomainIsActive(tmpvm) && -            tmpvm->monitorWatch == watch) { -            vm = tmpvm; -            break; -        } -        virDomainObjUnlock(tmpvm); -    } - -    if (!vm) -        goto cleanup; +    virDomainObjLock(vm); +    qemuDriverUnlock(driver); -    if (vm->monitor != fd) { +    if (vm->monitor != fd || vm->monitorWatch != watch) {         failed = 1;     } else {         if (events & (VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR)) @@ -2302,12 +2292,9 @@ qemudDispatchVMEvent(int watch, int fd, int events, void *opaque) {         }     } -cleanup: -    if (vm) -        virDomainObjUnlock(vm); +    virDomainObjUnlock(vm);     if (event)         qemuDomainEventQueue(driver, event); -    qemuDriverUnlock(driver);  }