[libvirt] [PATCH] qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix
A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows: Thread #1: qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove Thread #2: virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount The patch establishes a single order of taking locks: driver->domains list first, then a particular VM. This is done by implementing a set of virDomainObjListXxxLocked functions working with driver->domains that assume that list lock is already aquired by calling functions. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++-------- src/conf/virdomainobjlist.h | 9 ++++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++--- 4 files changed, 110 insertions(+), 17 deletions(-) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 7568f93..89e28a8 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -132,21 +132,19 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms, static virDomainObjPtr -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms, const unsigned char *uuid, bool ref) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj; - virObjectLock(doms); virUUIDFormat(uuid, uuidstr); obj = virHashLookup(doms->objs, uuidstr); - if (ref) { + if (ref) virObjectRef(obj); - virObjectUnlock(doms); - } + if (obj) { virObjectLock(obj); if (obj->removing) { @@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, obj = NULL; } } - if (!ref) - virObjectUnlock(doms); + return obj; +} + +static virDomainObjPtr +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, + const unsigned char *uuid, + bool ref) +{ + virDomainObjPtr obj; + + virObjectLock(doms); + obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref); + virObjectUnlock(doms); return obj; } @@ -169,6 +178,13 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms, return virDomainObjListFindByUUIDInternal(doms, uuid, false); } +virDomainObjPtr +virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms, + const unsigned char *uuid) +{ + return virDomainObjListFindByUUIDInternalLocked(doms, uuid, true); +} + virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms, @@ -177,6 +193,23 @@ virDomainObjListFindByUUIDRef(virDomainObjListPtr doms, return virDomainObjListFindByUUIDInternal(doms, uuid, true); } +virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms, + const char *name) +{ + virDomainObjPtr obj; + + obj = virHashLookup(doms->objsName, name); + virObjectRef(obj); + if (obj) { + virObjectLock(obj); + if (obj->removing) { + virObjectUnlock(obj); + virObjectUnref(obj); + obj = NULL; + } + } + return obj; +} virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, const char *name) @@ -312,14 +345,12 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, return ret; } - int -virDomainObjListRenameAddNew(virDomainObjListPtr doms, +virDomainObjListRenameAddNewLocked(virDomainObjListPtr doms, virDomainObjPtr vm, const char *name) { int ret = -1; - virObjectLock(doms); /* Add new name into the hash table of domain names. */ if (virHashAddEntry(doms->objsName, name, vm) < 0) @@ -332,21 +363,38 @@ virDomainObjListRenameAddNew(virDomainObjListPtr doms, ret = 0; cleanup: + return ret; +} + +int +virDomainObjListRenameAddNew(virDomainObjListPtr doms, + virDomainObjPtr vm, + const char *name) +{ + int ret = -1; + virObjectLock(doms); + ret = virDomainObjListRenameAddNewLocked(doms, vm, name); virObjectUnlock(doms); return ret; } +int +virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms, const char *name) +{ + virHashRemoveEntry(doms->objsName, name); + return 0; +} int virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name) { + int ret; virObjectLock(doms); - virHashRemoveEntry(doms->objsName, name); + ret = virDomainObjListRenameRemoveLocked(doms, name); virObjectUnlock(doms); - return 0; + return ret; } - /* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index f479598..bca31cf 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -38,8 +38,12 @@ virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms, const unsigned char *uuid); virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms, const unsigned char *uuid); +virDomainObjPtr virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms, + const unsigned char *uuid); virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, const char *name); +virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms, + const char *name); enum { VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0), @@ -54,8 +58,13 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms, int virDomainObjListRenameAddNew(virDomainObjListPtr doms, virDomainObjPtr vm, const char *name); +int virDomainObjListRenameAddNewLocked(virDomainObjListPtr doms, + virDomainObjPtr vm, + const char *name); int virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name); +int virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms, + const char *name); void virDomainObjListRemove(virDomainObjListPtr doms, virDomainObjPtr dom); void virDomainObjListRemoveLocked(virDomainObjListPtr doms, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5e05a98..b05d6cc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -881,8 +881,10 @@ virDomainObjListConvert; virDomainObjListExport; virDomainObjListFindByID; virDomainObjListFindByName; +virDomainObjListFindByNameLocked; virDomainObjListFindByUUID; virDomainObjListFindByUUIDRef; +virDomainObjListFindByUUIDRefLocked; virDomainObjListForEach; virDomainObjListGetActiveIDs; virDomainObjListGetInactiveNames; @@ -892,7 +894,9 @@ virDomainObjListNumOfDomains; virDomainObjListRemove; virDomainObjListRemoveLocked; virDomainObjListRenameAddNew; +virDomainObjListRenameAddNewLocked; virDomainObjListRenameRemove; +virDomainObjListRenameRemoveLocked; # cpu/cpu.h diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e46404b..b012516 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -206,6 +206,36 @@ struct qemuAutostartData { virConnectPtr conn; }; +/** + * qemuDomObjFromDomainLocked: + * @domain: Domain pointer that has to be looked up + * + * This function looks up @domain and returns the appropriate virDomainObjPtr + * that has to be released by calling virDomainObjEndAPI(). + * It assumes that driver->domains list is locked already, thus it uses Locked + * variant of virDomainObjListFindByUUIDRef function. + * + * Returns the domain object with incremented reference counter which is locked + * on success, NULL otherwise. + */ +static virDomainObjPtr +qemuDomObjFromDomainLocked(virDomainPtr domain) +{ + virDomainObjPtr vm; + virQEMUDriverPtr driver = domain->conn->privateData; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + vm = virDomainObjListFindByUUIDRefLocked(driver->domains, domain->uuid); + if (!vm) { + virUUIDFormat(domain->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, domain->name); + return NULL; + } + + return vm; +} /** * qemuDomObjFromDomain: @@ -19892,7 +19922,8 @@ static int qemuDomainRename(virDomainPtr dom, virCheckFlags(0, ret); - if (!(vm = qemuDomObjFromDomain(dom))) + virObjectLock(driver->domains); + if (!(vm = qemuDomObjFromDomainLocked(dom))) goto cleanup; if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0) @@ -19938,7 +19969,7 @@ static int qemuDomainRename(virDomainPtr dom, * internal error. And since new_name != name here, there's no * deadlock imminent. */ - tmp_dom = virDomainObjListFindByName(driver->domains, new_name); + tmp_dom = virDomainObjListFindByNameLocked(driver->domains, new_name); if (tmp_dom) { virObjectUnlock(tmp_dom); virObjectUnref(tmp_dom); @@ -19956,7 +19987,7 @@ static int qemuDomainRename(virDomainPtr dom, goto endjob; } - if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0) + if (virDomainObjListRenameAddNewLocked(driver->domains, vm, new_name) < 0) goto endjob; event_old = virDomainEventLifecycleNewFromObj(vm, @@ -19980,7 +20011,7 @@ static int qemuDomainRename(virDomainPtr dom, } /* Remove old domain name from table. */ - virDomainObjListRenameRemove(driver->domains, old_dom_name); + virDomainObjListRenameRemoveLocked(driver->domains, old_dom_name); event_new = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, @@ -20000,6 +20031,7 @@ static int qemuDomainRename(virDomainPtr dom, qemuDomainEventQueue(driver, event_old); qemuDomainEventQueue(driver, event_new); virObjectUnref(cfg); + virObjectUnlock(driver->domains); return ret; rollback: -- 1.8.3.1
On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows:
Thread #1:
qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove
Thread #2:
virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount
Ouch, the rename API should hold the list lock all the time it's doing something and not acquire it when any domain is locked. But you are duplicating a lot of code,
The patch establishes a single order of taking locks: driver->domains list first, then a particular VM. This is done by implementing a set of virDomainObjListXxxLocked functions working with driver->domains that assume that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++-------- src/conf/virdomainobjlist.h | 9 ++++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++--- 4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 7568f93..89e28a8 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -132,21 +132,19 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
static virDomainObjPtr -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms, const unsigned char *uuid, bool ref)
Indentation is off for renamed functions.
{ char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj;
- virObjectLock(doms); virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr); - if (ref) { + if (ref) virObjectRef(obj); - virObjectUnlock(doms); - } + if (obj) { virObjectLock(obj); if (obj->removing) { @@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, obj = NULL; } } - if (!ref) - virObjectUnlock(doms); + return obj; +} + +static virDomainObjPtr +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, + const unsigned char *uuid, + bool ref) +{ + virDomainObjPtr obj; + + virObjectLock(doms); + obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref); + virObjectUnlock(doms);
This won't work for ref == true as in that case the unlock is not the last thing done in virDomainObjListFindByUUIDInternalLocked(). I would rather we didn't invent bunch of new functions whose names aren't readable, but mayb even preferred open-coding some of them (or their parts) in the rename itself. The reasoning behind it is that rename is special API and it touches code that's not exposed normally. Or the other way would be moving parts of the rename code into this file to make it available for other drivers as well. Adding Michal to Cc as the original idea-haver =) to see if we can agree on that or I am mistaken and your approach is the way to go. Martin
26.01.2016 12:29, Martin Kletzander пишет:
On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows:
Thread #1:
qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove
Thread #2:
virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount
Ouch, the rename API should hold the list lock all the time it's doing something and not acquire it when any domain is locked.
But you are duplicating a lot of code, Not that much
The patch establishes a single order of taking locks: driver->domains list first, then a particular VM. This is done by implementing a set of virDomainObjListXxxLocked functions working with driver->domains that assume that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++-------- src/conf/virdomainobjlist.h | 9 ++++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++--- 4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 7568f93..89e28a8 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -132,21 +132,19 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
static virDomainObjPtr -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms, const unsigned char *uuid, bool ref)
Indentation is off for renamed functions.
Ok. I'll fix it if we decide to go on with my approach.
{ char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj;
- virObjectLock(doms); virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr); - if (ref) { + if (ref) virObjectRef(obj); - virObjectUnlock(doms); - } + if (obj) { virObjectLock(obj); if (obj->removing) { @@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, obj = NULL; } } - if (!ref) - virObjectUnlock(doms); + return obj; +} + +static virDomainObjPtr +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, + const unsigned char *uuid, + bool ref) +{ + virDomainObjPtr obj; + + virObjectLock(doms); + obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref); + virObjectUnlock(doms);
This won't work for ref == true as in that case the unlock is not the last thing done in virDomainObjListFindByUUIDInternalLocked().
I still think that this won't cause any problem as far as list lock is aquired. Otherwise we would have seen them in virDomainObjListNumOfDomains
I would rather we didn't invent bunch of new functions whose names aren't readable, but mayb even preferred open-coding some of them (or their parts) in the rename itself. The reasoning behind it is that rename is special API and it touches code that's not exposed normally. Or the other way would be moving parts of the rename code into this file to make it available for other drivers as well.
I don't insist on the way I'm proposing, let the patch be the reference of the problem.
Adding Michal to Cc as the original idea-haver =) to see if we can agree on that or I am mistaken and your approach is the way to go.
Martin
On Tue, Jan 26, 2016 at 01:31:25PM +0300, Maxim Nestratov wrote:
26.01.2016 12:29, Martin Kletzander пишет:
On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows:
Thread #1:
qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove
Thread #2:
virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount
Ouch, the rename API should hold the list lock all the time it's doing something and not acquire it when any domain is locked.
But you are duplicating a lot of code, Not that much
I meant to edit that out after reading the whole e-mail, my apologies.
The patch establishes a single order of taking locks: driver->domains list first, then a particular VM. This is done by implementing a set of virDomainObjListXxxLocked functions working with driver->domains that assume that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++-------- src/conf/virdomainobjlist.h | 9 ++++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++--- 4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 7568f93..89e28a8 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -132,21 +132,19 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
static virDomainObjPtr -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms, const unsigned char *uuid, bool ref)
Indentation is off for renamed functions.
Ok. I'll fix it if we decide to go on with my approach.
{ char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj;
- virObjectLock(doms); virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr); - if (ref) { + if (ref) virObjectRef(obj); - virObjectUnlock(doms); - } + if (obj) { virObjectLock(obj); if (obj->removing) { @@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, obj = NULL; } } - if (!ref) - virObjectUnlock(doms); + return obj; +} + +static virDomainObjPtr +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, + const unsigned char *uuid, + bool ref) +{ + virDomainObjPtr obj; + + virObjectLock(doms); + obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref); + virObjectUnlock(doms);
This won't work for ref == true as in that case the unlock is not the last thing done in virDomainObjListFindByUUIDInternalLocked().
I still think that this won't cause any problem as far as list lock is aquired. Otherwise we would have seen them in virDomainObjListNumOfDomains
That's kind of why there's the difference between ref=true and ref=false. The thing is that ref=false is prone to deadlocks even though they are not as easy to reproduce. We'll see what the decision will be. Martin
On 25.01.2016 10:16, Maxim Nestratov wrote:
A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows:
Thread #1:
qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove
Thread #2:
virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount
The patch establishes a single order of taking locks: driver->domains list first, then a particular VM. This is done by implementing a set of virDomainObjListXxxLocked functions working with driver->domains that assume that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++-------- src/conf/virdomainobjlist.h | 9 ++++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++--- 4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e46404b..b012516 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -206,6 +206,36 @@ struct qemuAutostartData { virConnectPtr conn; };
+/** + * qemuDomObjFromDomainLocked: + * @domain: Domain pointer that has to be looked up + * + * This function looks up @domain and returns the appropriate virDomainObjPtr + * that has to be released by calling virDomainObjEndAPI(). + * It assumes that driver->domains list is locked already, thus it uses Locked + * variant of virDomainObjListFindByUUIDRef function. + * + * Returns the domain object with incremented reference counter which is locked + * on success, NULL otherwise. + */ +static virDomainObjPtr +qemuDomObjFromDomainLocked(virDomainPtr domain) +{ + virDomainObjPtr vm; + virQEMUDriverPtr driver = domain->conn->privateData; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + vm = virDomainObjListFindByUUIDRefLocked(driver->domains, domain->uuid); + if (!vm) { + virUUIDFormat(domain->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s' (%s)"), + uuidstr, domain->name); + return NULL; + } + + return vm; +}
/** * qemuDomObjFromDomain: @@ -19892,7 +19922,8 @@ static int qemuDomainRename(virDomainPtr dom,
virCheckFlags(0, ret);
- if (!(vm = qemuDomObjFromDomain(dom))) + virObjectLock(driver->domains);
This is rather ugly. While driver->domains is a lockable object, it's internals are intentionally hidden from the rest of the code so that nobody from outside should touch them. That's why we have locking APIs over virDomainObjList object. I know this will work, I just find it hackish. However, I find your approach understandable and kind of agree with it. What we should do is: 1) lock list 2) lookup domain 3) begin MODIFY job on the domain (*) 4) rename the domain (here are hidden all the checks, moving the XML file, etc. - basically everything from the qemuDomainRename()) 5) end job 6) unlock domain 7) unlock list * - here we will need BeginJob() without timeout - we don't want to keep the list locked longer than needed. Either setting the job is done instantly or not at all. What if, instead of introducing bunch of Locked() functions we introduce a new internal API to virDomainObjList that will look like this: int virDomainObjListRename(virDomainObjListPtr domains, virDomainObjPtr dom, const char *new_name, int (*driverRename)(virDomainObjPtr, ...), int (*rollBack)(virDomainObjPtr, ...)); This function will encapsulate the renaming and at the correct spot it will call driverRename() so that driver can adjust its internal state. If the driver is successful, just remove the old entry. If it is not, call rollBack() callback which will cancel partially performed operation and restore original state of the driver. Moreover, in the virDomainObjListRename() we can ensure the locking order and we don't need to introduce new BeginJob() API with no timeout. Michal
On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
On 25.01.2016 10:16, Maxim Nestratov wrote: ...
virCheckFlags(0, ret);
- if (!(vm = qemuDomObjFromDomain(dom))) + virObjectLock(driver->domains);
This is rather ugly. While driver->domains is a lockable object, it's internals are intentionally hidden from the rest of the code so that nobody from outside should touch them. That's why we have locking APIs over virDomainObjList object. I know this will work, I just find it hackish.
Yeah, I don't like this either.
However, I find your approach understandable and kind of agree with it. What we should do is:
1) lock list 2) lookup domain 3) begin MODIFY job on the domain (*)
4) rename the domain (here are hidden all the checks, moving the XML file, etc. - basically everything from the qemuDomainRename())
5) end job 6) unlock domain 7) unlock list
* - here we will need BeginJob() without timeout - we don't want to keep the list locked longer than needed. Either setting the job is done instantly or not at all.
This sounds very ugly and fragile too.
What if, instead of introducing bunch of Locked() functions we introduce a new internal API to virDomainObjList that will look like this:
int virDomainObjListRename(virDomainObjListPtr domains, virDomainObjPtr dom, const char *new_name, int (*driverRename)(virDomainObjPtr, ...), int (*rollBack)(virDomainObjPtr, ...));
This function will encapsulate the renaming and at the correct spot it will call driverRename() so that driver can adjust its internal state. If the driver is successful, just remove the old entry. If it is not, call rollBack() callback which will cancel partially performed operation and restore original state of the driver. Moreover, in the virDomainObjListRename() we can ensure the locking order and we don't need to introduce new BeginJob() API with no timeout.
But very much this approach I like :-) Jirka
On Tue, Jan 26, 2016 at 02:14:23PM +0100, Jiri Denemark wrote:
On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
On 25.01.2016 10:16, Maxim Nestratov wrote: ...
virCheckFlags(0, ret);
- if (!(vm = qemuDomObjFromDomain(dom))) + virObjectLock(driver->domains);
This is rather ugly. While driver->domains is a lockable object, it's internals are intentionally hidden from the rest of the code so that nobody from outside should touch them. That's why we have locking APIs over virDomainObjList object. I know this will work, I just find it hackish.
Yeah, I don't like this either.
However, I find your approach understandable and kind of agree with it. What we should do is:
1) lock list 2) lookup domain 3) begin MODIFY job on the domain (*)
4) rename the domain (here are hidden all the checks, moving the XML file, etc. - basically everything from the qemuDomainRename())
5) end job 6) unlock domain 7) unlock list
* - here we will need BeginJob() without timeout - we don't want to keep the list locked longer than needed. Either setting the job is done instantly or not at all.
This sounds very ugly and fragile too.
What if, instead of introducing bunch of Locked() functions we introduce a new internal API to virDomainObjList that will look like this:
int virDomainObjListRename(virDomainObjListPtr domains, virDomainObjPtr dom, const char *new_name, int (*driverRename)(virDomainObjPtr, ...), int (*rollBack)(virDomainObjPtr, ...));
This function will encapsulate the renaming and at the correct spot it will call driverRename() so that driver can adjust its internal state. If the driver is successful, just remove the old entry. If it is not, call rollBack() callback which will cancel partially performed operation and restore original state of the driver. Moreover, in the virDomainObjListRename() we can ensure the locking order and we don't need to introduce new BeginJob() API with no timeout.
But very much this approach I like :-)
That's what I meant with: "Or the other way would be moving parts of the rename code into this file to make it available for other drivers as well." but you explained it in more detail and understandably. ACK for that idea as well.
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
26.01.2016 16:33, Martin Kletzander пишет:
On Tue, Jan 26, 2016 at 02:14:23PM +0100, Jiri Denemark wrote:
On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
On 25.01.2016 10:16, Maxim Nestratov wrote: ... [snip] What if, instead of introducing bunch of Locked() functions we introduce a new internal API to virDomainObjList that will look like this:
int virDomainObjListRename(virDomainObjListPtr domains, virDomainObjPtr dom, const char *new_name, int (*driverRename)(virDomainObjPtr, ...), int (*rollBack)(virDomainObjPtr, ...));
This function will encapsulate the renaming and at the correct spot it will call driverRename() so that driver can adjust its internal state. If the driver is successful, just remove the old entry. If it is not, call rollBack() callback which will cancel partially performed operation and restore original state of the driver. Moreover, in the virDomainObjListRename() we can ensure the locking order and we don't need to introduce new BeginJob() API with no timeout.
But very much this approach I like :-)
That's what I meant with: "Or the other way would be moving parts of the rename code into this file to make it available for other drivers as well."
but you explained it in more detail and understandably. ACK for that idea as well.
I like it too. I can rework the patch following this way if no one wants to volunteer.
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (4)
-
Jiri Denemark -
Martin Kletzander -
Maxim Nestratov -
Michal Privoznik