On Mon, Feb 04, 2013 at 05:22:59PM +0100, Jiri Denemark wrote:
On Fri, Feb 01, 2013 at 11:18:27 +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Switch virDomainObjList to inherit from virObjectLockable and make all the APIs acquire/release the mutex when running. This makes virDomainObjList completely self-locking and no longer reliant on the hypervisor driver locks --- src/conf/domain_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 79da5eb..a1e899f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -60,7 +60,7 @@ verify(VIR_DOMAIN_VIRT_LAST <= 32);
struct _virDomainObjList { - virObject parent; + virObjectLockable parent;
/* uuid string -> virDomainObj mapping * for O(1), lockless lookup-by-uuid */ @@ -715,7 +715,7 @@ static int virDomainObjOnceInit(void) virDomainObjDispose))) return -1;
- if (!(virDomainObjListClass = virClassNew(virClassForObject(), + if (!(virDomainObjListClass = virClassNew(virClassForObjectLockable(), "virDomainObjList", sizeof(virDomainObjList), virDomainObjListDispose)))
These two hunks should go to 3/13.
...
@@ -1880,25 +1886,32 @@ void virDomainObjAssignDef(virDomainObjPtr domain, * current def is Live * */ -virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, - virCapsPtr caps, - const virDomainDefPtr def, - unsigned int flags, - virDomainDefPtr *oldDef) +static virDomainObjPtr +virDomainObjListAddLocked(virDomainObjListPtr doms, + virCapsPtr caps, + const virDomainDefPtr def, + unsigned int flags, + virDomainDefPtr *oldDef) { virDomainObjPtr vm; char uuidstr[VIR_UUID_STRING_BUFLEN]; + if (oldDef) *oldDef = false;
+ virUUIDFormat(def->uuid, uuidstr); + /* See if a VM with matching UUID already exists */ - if ((vm = virDomainObjListFindByUUID(doms, def->uuid))) { + if ((vm = virHashLookup(doms->objs, uuidstr))) { + virObjectLock(vm); /* UUID matches, but if names don't match, refuse it */ if (STRNEQ(vm->def->name, def->name)) { virUUIDFormat(vm->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("domain '%s' is already defined with uuid %s"), vm->def->name, uuidstr); + virObjectUnlock(vm); + vm = NULL;
These two lines should go to 4/13.
goto cleanup; }
@@ -1908,6 +1921,8 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, virReportError(VIR_ERR_OPERATION_INVALID, _("domain is already active as '%s'"), vm->def->name); + virObjectUnlock(vm); + vm = NULL; goto cleanup; } }
And this hunk should go to 4/13 too.
@@ -1934,12 +1949,11 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, } } else { /* UUID does not match, but if a name matches, refuse it */ - if ((vm = virDomainObjListFindByName(doms, def->name))) { + if ((vm = virHashSearch(doms->objs, virDomainObjListSearchName, def->name))) {
I believe you wanted to add virObjectLock(vm) here rather than...
virUUIDFormat(vm->def->uuid, uuidstr); virReportError(VIR_ERR_OPERATION_FAILED, _("domain '%s' already exists with uuid %s"), def->name, uuidstr); - virObjectUnlock(vm);
...removing this unlock here.
Yes, that is correct. I've made that change Given that all other comments are just moving chunks to previous patches, do you want to see a v2 ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|