On 01/25/2017 05:37 PM, John Ferlan wrote:
> Originally/discovered proposed by "Wang King <king.wang@xxxxxxxxxx>"
>
> When the virCloseCallbacksSet is first called, it increments the refcnt
> on the domain object to ensure it doesn't get deleted before the callback
> is called. The refcnt would be decremented in virCloseCallbacksUnset once
> the entry is removed from the closeCallbacks has table.
>
> When (mostly) normal shutdown occurs, the qemuProcessStop will end up
> calling qemuProcessAutoDestroyRemove and will remove the callback from
> the list and hash table normally and decrement the refcnt.
>
> However, when qemuConnectClose calls virCloseCallbacksRun, it will scan
> the (locked) closeCallbacks list for matching domain and callback function.
> If an entry is found, it will be removed from the closeCallbacks list and
> placed into a lookaside list to be processed when the closeCallbacks lock
> is dropped. The callback function (e.g. qemuProcessAutoDestroy) is called
> and will run qemuProcessStop. That code will fail to find the callback
> in the list when qemuProcessAutoDestroyRemove is called and thus not decrement
> the domain refcnt. Instead since the entry isn't found the code will just
> return (mostly) harmlessly.
>
> This patch will resolve the issue by taking another ref during the
> search UUID process during virCloseCallackRun, decrementing the refcnt
> taken by virCloseCallbacksSet, calling the callback routine and returning
> overwriting the vm (since it could return NULL). Finally, it will call the
> virDomainObjEndAPI to lower the refcnt and remove the lock taken during
> the search UUID processing. This may cause the vm to be destroyed.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/util/virclosecallbacks.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
> index 2f430cf..4db50e8 100644
> --- a/src/util/virclosecallbacks.c
> +++ b/src/util/virclosecallbacks.c
> @@ -346,17 +346,24 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks,
> for (i = 0; i < list->nentries; i++) {
> virDomainObjPtr vm;
>
> - if (!(vm = virDomainObjListFindByUUID(domains,
> - list->entries[i].uuid))) {
> + /* Grab a ref and lock to the vm */
> + if (!(vm = virDomainObjListFindByUUIDRef(domains,
> + list->entries[i].uuid))) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virUUIDFormat(list->entries[i].uuid, uuidstr);
> VIR_DEBUG("No domain object with UUID %s", uuidstr);
> continue;
> }
>
> - vm = list->entries[i].callback(vm, conn, opaque);
> - if (vm)
> - virObjectUnlock(vm);
> + /* Remove the ref taken out during virCloseCallbacksSet since
> + * we're about to call the callback function and we have another
> + * ref anyway (so it cannot be deleted).
> + *
> + * Call the callback function, ignoring the return since it might be
> + * NULL. Once we're done with the object, then end the API usage. */
> + virObjectUnref(vm);
> + ignore_value(list->entries[i].callback(vm, conn, opaque));
> + virDomainObjEndAPI(&vm);
> }
> VIR_FREE(list->entries);
> VIR_FREE(list);
>
Looks very hacky, but I am unable to come up with a better fix.
ACK
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
