virObject is the base struct that manages reference-counting for all structs that need the ability of reference-counting. virAtomic provides atomic operations which are thread-safe. --- src/Makefile.am | 2 + src/libvirt_private.syms | 5 ++++ src/util/viratomic.c | 46 ++++++++++++++++++++++++++++++++++++++++ src/util/viratomic.h | 30 ++++++++++++++++++++++++++ src/util/virobject.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virobject.h | 39 ++++++++++++++++++++++++++++++++++ 6 files changed, 174 insertions(+), 0 deletions(-) create mode 100644 src/util/viratomic.c create mode 100644 src/util/viratomic.h create mode 100644 src/util/virobject.c create mode 100644 src/util/virobject.h diff --git a/src/Makefile.am b/src/Makefile.am index 9b54679..9e74060 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -81,6 +81,8 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/xml.c util/xml.h \ util/virtaudit.c util/virtaudit.h \ + util/viratomic.c util/viratomic.h \ + util/virobject.c util/virobject.h \ util/virterror.c util/virterror_internal.h EXTRA_DIST += util/threads-pthread.c util/threads-win32.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65a86d3..98f8d6f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -993,3 +993,8 @@ virXPathUInt; virXPathULong; virXPathULongHex; virXPathULongLong; + +# object.h +virObjectInit; +virObjectRef; +virObjectUnref; diff --git a/src/util/viratomic.c b/src/util/viratomic.c new file mode 100644 index 0000000..629f189 --- /dev/null +++ b/src/util/viratomic.c @@ -0,0 +1,46 @@ +/* + * viratomic.c: atomic operations + * + * Copyright (C) 2011 Hu Tao + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Hu Tao <hutao(a)cn.fujitsu.com&gt; + */ + +#include "viratomic.h" + +#ifdef WIN32 +long virAtomicInc(long *value) +{ + return InterlockedIncrement(value); +} + +long virAtomicDec(long *value) +{ + return InterlockedDecrement(value); +} +#else /* WIN32 */ +long virAtomicInc(long *value) +{ + return __sync_add_and_fetch(value, 1); +} + +long virAtomicDec(long *value) +{ + return __sync_sub_and_fetch(value, 1); +} +#endif /* WIN32 */ diff --git a/src/util/viratomic.h b/src/util/viratomic.h new file mode 100644 index 0000000..a10cb65 --- /dev/null +++ b/src/util/viratomic.h @@ -0,0 +1,30 @@ +/* + * viratomic.h: atomic operations + * + * Copyright (C) 2011 Hu Tao + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Hu Tao <hutao(a)cn.fujitsu.com&gt; + */ + +#ifndef __VIR_ATOMIC_H +#define __VIR_ATOMIC_H + +long virAtomicInc(long *value); +long virAtomicDec(long *value); + +#endif /* __VIR_ATOMIC_H */ diff --git a/src/util/virobject.c b/src/util/virobject.c new file mode 100644 index 0000000..aeedbef --- /dev/null +++ b/src/util/virobject.c @@ -0,0 +1,52 @@ +/* + * virobject.c: base object that manages reference-counting + * + * Copyright (C) 2011 Hu Tao + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Hu Tao <hutao(a)cn.fujitsu.com&gt; + */ + +#include "viratomic.h" +#include "virobject.h" +#include "logging.h" + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)) +{ + if (!free) { + VIR_ERROR0("method free is required."); + return -1; + } + + obj->ref = 1; + obj->free = free; + + return 0; +} + +void virObjectRef(virObjectPtr obj) +{ + sa_assert(obj->ref > 0); + virAtomicInc(&obj->ref); +} + +void virObjectUnref(virObjectPtr obj) +{ + sa_assert(obj->ref > 0); + if (virAtomicDec(&obj->ref) == 0) + obj->free(obj); +} diff --git a/src/util/virobject.h b/src/util/virobject.h new file mode 100644 index 0000000..cd7d3e8 --- /dev/null +++ b/src/util/virobject.h @@ -0,0 +1,39 @@ +/* + * virobject.h: base object that manages reference-counting + * + * Copyright (C) 2011 Hu Tao + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Hu Tao <hutao(a)cn.fujitsu.com&gt; + */ + +#ifndef __VIR_OBJECT_H +#define __VIR_OBJECT_H + +typedef struct _virObject virObject; +typedef virObject *virObjectPtr; + +struct _virObject { + long ref; + void (*free)(virObjectPtr obj); +}; + +int virObjectInit(virObjectPtr obj, void (*free)(virObjectPtr obj)); +void virObjectRef(virObjectPtr obj); +void virObjectUnref(virObjectPtr obj); + +#endif /* __VIR_OBJECT_H */ -- 1.7.3.1 -- Thanks, Hu Tao

On Wed, Apr 06, 2011 at 02:19:57PM -0600, Eric Blake wrote: Oops, there are always things escape from under your nose:( Why the linker doesn't complain about this this time? Yes. Not sure about this. I don't have a mingw environment to test, but I trust gcc and guess it does. Agreed, this is a better way. OK. No, there are cases you just want to inc/dec a value but do not check the modified value. OK. Agreed. And it is dangerous to initialize an object more than once because the ref count may already changed and a second initializaton just do the wrong thing. This means bad things happened, and it is not enough to just give a warning. Think about this: two threads visit an object whose memory is corrupted that it's ref count becomes 0 but it doesn't get freed. thread 1: thread 2: ref the object (ref count becomes 1) unref the object (ref count becomes 0, object being freed) do something with the object, but it is already freed by thread 2 We should catch abnormal ref count by some way(if not sa_assert) Exactly the way(you have already saw my other patches when I was typing this:)) OK. No. It is meaningless to return the current reference count, because it may already changed by others after the caller reads the returned value on which it is not safe to make some choice.

This prepares for the next patch. The bad is we have no way to check the return value for CreateMutex when it is used as a static initializer. --- src/util/threads-pthread.h | 5 +++++ src/util/threads-win32.h | 5 +++++ 2 files changed, 10 insertions(+), 0 deletions(-) diff --git a/src/util/threads-pthread.h b/src/util/threads-pthread.h index b25d0c2..ff50253 100644 --- a/src/util/threads-pthread.h +++ b/src/util/threads-pthread.h @@ -23,6 +23,11 @@ #include <pthread.h> +#define VIR_MUTEX_INITIALIZER \ +{ \ + .lock = PTHREAD_MUTEX_INITIALIZER \ +} + struct virMutex { pthread_mutex_t lock; }; diff --git a/src/util/threads-win32.h b/src/util/threads-win32.h index bb7c455..cfadbe4 100644 --- a/src/util/threads-win32.h +++ b/src/util/threads-win32.h @@ -23,6 +23,11 @@ #include <windows.h> +#define VIR_MUTEX_INITIALIZER \ +{ \ + .lock = CreateMutex(NULL, FALSE, NULL) \ +} + struct virMutex { HANDLE lock; }; -- 1.7.3.1

On 04/20/2011 03:15 AM, Hu Tao wrote: Swap these two sections (chunks are alphabetic by header), as well as swap these two lines (within a header, lines are alphabetic). Hmm, if I implement virOnce first, maybe we should use that instead. Clunky, but does the job ;) Either both failures should abort(), or both should return -1. They are both programming errors, so I'm personally okay with abort(), but this would be the first use of abort() outside gnulib files, so I'll see if either Dan has opinions as well. In fact, if we go with abort() in both places, then this can return void instead of int. Likewise, by adding ATTRIBUTE_NONNULL(2) to the prototype, then the compiler (well, at least clang) will help catch coding errors of failing to pass a callback function. Oops - you used TAB instead of space; 'make syntax-check' should have caught that. If we keep the abort, then we _need_ the VIR_ERROR before hand saying why we quit. Again, don't ever abort() without VIR_ERROR stating why. And maybe we should think about installing a SIGABRT handler so that aborts print a nicer message for the end user? I dislike extra casts, and this signature forces every client to have an explicit cast in their free function. Could we instead make this: typedef void (*virObjectFree)(void *obj); with some additional fallout documented below? Also, can we attach ATTRIBUTE_NONNULL(1) to a typedef? (I'm not quite sure how that works with gcc). s/guarenteed/guaranteed to/ The argument to fooFree will be &obj, which in turn can be cast as struct foo* because obj is the first member. struct foo * fooCreate() { struct foo *myfoo; if (VIR_ALLOC(myfoo) < 0) { virReportOOMError(); return NULL; } virObjectInit(&myfoo->obj, fooFree); ... // all other contents of myfoo return myfoo; } 4. implement the free method for your struct like this: static void fooFree(void *obj) { struct foo *myfoo = obj; ... // all other contents of myfoo VIR_FREE(myfoo); } ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(1) -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

--- daemon/dispatch.c | 2 - daemon/libvirtd.c | 62 +++++++++++++++++++++++++++++++++++++++------------- daemon/libvirtd.h | 5 ++- 3 files changed, 49 insertions(+), 20 deletions(-) diff --git a/daemon/dispatch.c b/daemon/dispatch.c index 4814017..b2d6927 100644 --- a/daemon/dispatch.c +++ b/daemon/dispatch.c @@ -524,9 +524,7 @@ remoteDispatchClientCall (struct qemud_server *server, */ rv = (data->fn)(server, client, conn, &msg->hdr, &rerr, &args, &ret); - virMutexLock(&server->lock); virMutexLock(&client->lock); - virMutexUnlock(&server->lock); xdr_free (data->args_filter, (char*)&args); diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 42cbe5d..d3ba97b 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -270,6 +270,8 @@ static void sig_fatal(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED, static void qemudDispatchClientEvent(int watch, int fd, int events, void *opaque); static void qemudDispatchServerEvent(int watch, int fd, int events, void *opaque); static int qemudStartWorker(struct qemud_server *server, struct qemud_worker *worker); +static void qemudClientRef(struct qemud_client *client); +static void qemudClientUnref(struct qemud_client *client); void qemudClientMessageQueuePush(struct qemud_client_message **queue, @@ -1338,6 +1340,8 @@ int qemudGetSocketIdentity(int fd, uid_t *uid, pid_t *pid) { #endif +static void qemudFreeClient(virObjectPtr obj); + static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket *sock) { int fd; virSocketAddr addr; @@ -1411,6 +1415,10 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket VIR_ERROR0(_("cannot initialize mutex")); goto error; } + if (virObjectInit(&client->obj, qemudFreeClient)) { + VIR_ERROR0(_("cannot initialize virobject")); + goto error; + } client->magic = QEMUD_CLIENT_MAGIC; client->fd = fd; @@ -1576,12 +1584,10 @@ static struct qemud_client *qemudPendingJob(struct qemud_server *server) { int i; for (i = 0 ; i < server->nclients ; i++) { - virMutexLock(&server->clients[i]->lock); if (server->clients[i]->dx) { - /* Delibrately don't unlock client - caller wants the lock */ + qemudClientRef(server->clients[i]); return server->clients[i]; } - virMutexUnlock(&server->clients[i]->lock); } return NULL; } @@ -1604,31 +1610,36 @@ static void *qemudWorker(void *data) } } if (worker->quitRequest) { - virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); + qemudClientUnref(client); return NULL; } worker->processingCall = 1; virMutexUnlock(&server->lock); - /* We own a locked client now... */ - client->refs++; + virMutexLock(&client->lock); /* Remove our message from dispatch queue while we use it */ msg = qemudClientMessageQueueServe(&client->dx); + if (!msg) { + virMutexUnlock(&client->lock); + qemudClientUnref(client); + continue; + } + /* This function drops the lock during dispatch, * and re-acquires it before returning */ if (remoteDispatchClientRequest (server, client, msg) < 0) { VIR_FREE(msg); qemudDispatchClientFailure(client); - client->refs--; virMutexUnlock(&client->lock); + qemudClientUnref(client); continue; } - client->refs--; virMutexUnlock(&client->lock); + qemudClientUnref(client); virMutexLock(&server->lock); worker->processingCall = 0; @@ -2155,6 +2166,7 @@ qemudDispatchClientEvent(int watch, int fd, int events, void *opaque) { virMutexLock(&server->clients[i]->lock); if (server->clients[i]->watch == watch) { client = server->clients[i]; + qemudClientRef(client); break; } virMutexUnlock(&server->clients[i]->lock); @@ -2168,6 +2180,7 @@ qemudDispatchClientEvent(int watch, int fd, int events, void *opaque) { if (client->fd != fd) { virMutexUnlock(&client->lock); + qemudClientUnref(client); return; } @@ -2190,6 +2203,7 @@ qemudDispatchClientEvent(int watch, int fd, int events, void *opaque) { qemudDispatchClientFailure(client); virMutexUnlock(&client->lock); + qemudClientUnref(client); } @@ -2306,7 +2320,9 @@ static void qemudInactiveTimer(int timerid, void *data) { } } -static void qemudFreeClient(struct qemud_client *client) { +static void qemudFreeClient(virObjectPtr obj) { + struct qemud_client *client = (struct qemud_client *)obj; + while (client->rx) { struct qemud_client_message *msg = qemudClientMessageQueueServe(&client->rx); @@ -2333,6 +2349,16 @@ static void qemudFreeClient(struct qemud_client *client) { VIR_FREE(client); } +static void qemudClientRef(struct qemud_client *client) +{ + virObjectRef(&client->obj); +} + +static void qemudClientUnref(struct qemud_client *client) +{ + virObjectUnref(&client->obj); +} + static void *qemudRunLoop(void *opaque) { struct qemud_server *server = opaque; int timerid = -1; @@ -2396,13 +2422,17 @@ static void *qemudRunLoop(void *opaque) { reprocess: for (i = 0 ; i < server->nclients ; i++) { - int inactive; + struct qemud_client *client = NULL; virMutexLock(&server->clients[i]->lock); - inactive = server->clients[i]->fd == -1 - && server->clients[i]->refs == 0; - virMutexUnlock(&server->clients[i]->lock); - if (inactive) { - qemudFreeClient(server->clients[i]); + if (server->clients[i]->fd == -1) { + client = server->clients[i]; + server->clients[i] = NULL; + } + if (!client) { + virMutexUnlock(&server->clients[i]->lock); + } else { + virMutexUnlock(&client->lock); + qemudClientUnref(client); server->nclients--; if (i < server->nclients) memmove(server->clients + i, @@ -2451,7 +2481,7 @@ cleanup: } VIR_FREE(server->workers); for (i = 0; i < server->nclients; i++) - qemudFreeClient(server->clients[i]); + qemudClientUnref(server->clients[i]); server->nclients = 0; VIR_SHRINK_N(server->clients, server->nclients_max, server->nclients_max); diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h index d37c3fd..5ac4eb3 100644 --- a/daemon/libvirtd.h +++ b/daemon/libvirtd.h @@ -49,6 +49,7 @@ # include "logging.h" # include "threads.h" # include "network.h" +# include "virobject.h" # if WITH_DTRACE # ifndef LIBVIRTD_PROBES_H @@ -186,6 +187,8 @@ struct qemud_client_stream { /* Stores the per-client connection state */ struct qemud_client { + virObject obj; /* should be the first member */ + virMutex lock; int magic; @@ -243,8 +246,6 @@ struct qemud_client { * called, it will be set back to NULL if that succeeds. */ virConnectPtr conn; - int refs; - }; # define QEMUD_CLIENT_MAGIC 0x7788aaee -- 1.7.3.1 -- Thanks, Hu Tao

This patch also eliminates a dead-lock bug in qemuDomainObjBeginJobWithDriver: if virCondWaitUntil() timeouts, the thread tries to acquire qemu driver lock while holding virDomainObj lock. --- src/conf/domain_conf.c | 56 ++++---- src/conf/domain_conf.h | 6 +- src/openvz/openvz_conf.c | 8 +- src/qemu/qemu_domain.c | 32 ++--- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 304 ++++++++++++++++++++------------------------- src/qemu/qemu_migration.c | 45 +++---- src/qemu/qemu_process.c | 33 ++--- src/vmware/vmware_conf.c | 2 +- 9 files changed, 215 insertions(+), 273 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 90a1317..fc76a00 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -47,6 +47,7 @@ #include "storage_file.h" #include "files.h" #include "bitmap.h" +#include "virobject.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -395,9 +396,7 @@ static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { virDomainObjPtr obj = payload; - virDomainObjLock(obj); - if (virDomainObjUnref(obj) > 0) - virDomainObjUnlock(obj); + virDomainObjUnref(obj); } int virDomainObjListInit(virDomainObjListPtr doms) @@ -437,7 +436,7 @@ virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms, virDomainObjPtr obj; obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id); if (obj) - virDomainObjLock(obj); + virDomainObjRef(obj); return obj; } @@ -452,7 +451,7 @@ virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms, obj = virHashLookup(doms->objs, uuidstr); if (obj) - virDomainObjLock(obj); + virDomainObjRef(obj); return obj; } @@ -476,7 +475,7 @@ virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms, virDomainObjPtr obj; obj = virHashSearch(doms->objs, virDomainObjListSearchName, name); if (obj) - virDomainObjLock(obj); + virDomainObjRef(obj); return obj; } @@ -967,6 +966,12 @@ static void virDomainObjFree(virDomainObjPtr dom) { if (!dom) return; + virDomainObjUnref(dom); +} + +static void doDomainObjFree(virObjectPtr obj) +{ + virDomainObjPtr dom = (virDomainObjPtr)obj; VIR_DEBUG("obj=%p", dom); virDomainDefFree(dom->def); @@ -984,21 +989,13 @@ static void virDomainObjFree(virDomainObjPtr dom) void virDomainObjRef(virDomainObjPtr dom) { - dom->refs++; - VIR_DEBUG("obj=%p refs=%d", dom, dom->refs); + virObjectRef(&dom->obj); } -int virDomainObjUnref(virDomainObjPtr dom) +void virDomainObjUnref(virDomainObjPtr dom) { - dom->refs--; - VIR_DEBUG("obj=%p refs=%d", dom, dom->refs); - if (dom->refs == 0) { - virDomainObjUnlock(dom); - virDomainObjFree(dom); - return 0; - } - return dom->refs; + virObjectUnref(&dom->obj); } static virDomainObjPtr virDomainObjNew(virCapsPtr caps) @@ -1010,6 +1007,11 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps) return NULL; } + if (virObjectInit(&domain->obj, doDomainObjFree)) { + VIR_FREE(domain); + return NULL; + } + if (caps->privateDataAllocFunc && !(domain->privateData = (caps->privateDataAllocFunc)())) { virReportOOMError(); @@ -1027,9 +1029,7 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps) return NULL; } - virDomainObjLock(domain); domain->state = VIR_DOMAIN_SHUTOFF; - domain->refs = 1; virDomainSnapshotObjListInit(&domain->snapshots); @@ -1075,8 +1075,10 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps, domain->def = def; virUUIDFormat(def->uuid, uuidstr); + virDomainObjRef(domain); if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) { - VIR_FREE(domain); + virDomainObjUnref(domain); + virDomainObjFree(domain); return NULL; } @@ -1149,9 +1151,7 @@ virDomainObjGetPersistentDef(virCapsPtr caps, } /* - * The caller must hold a lock on the driver owning 'doms', - * and must also have locked 'dom', to ensure no one else - * is either waiting for 'dom' or still usingn it + * The caller must hold a lock on the driver owning 'doms'. */ void virDomainRemoveInactive(virDomainObjListPtr doms, virDomainObjPtr dom) @@ -1159,9 +1159,8 @@ void virDomainRemoveInactive(virDomainObjListPtr doms, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->def->uuid, uuidstr); - virDomainObjUnlock(dom); - virHashRemoveEntry(doms->objs, uuidstr); + virDomainObjUnref(dom); } @@ -6146,7 +6145,7 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps, error: /* obj was never shared, so unref should return 0 */ - ignore_value(virDomainObjUnref(obj)); + virDomainObjUnref(obj); return NULL; } @@ -8449,10 +8448,12 @@ static virDomainObjPtr virDomainLoadConfig(virCapsPtr caps, if (!(dom = virDomainAssignDef(caps, doms, def, false))) goto error; + virDomainObjLock(dom); dom->autostart = autostart; if (notify) (*notify)(dom, newVM, opaque); + virDomainObjUnlock(dom); VIR_FREE(configFile); VIR_FREE(autostartLink); @@ -8501,9 +8502,8 @@ static virDomainObjPtr virDomainLoadStatus(virCapsPtr caps, return obj; error: - /* obj was never shared, so unref should return 0 */ if (obj) - ignore_value(virDomainObjUnref(obj)); + virDomainObjUnref(obj); VIR_FREE(statusFile); return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 10e73cb..3218672 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -40,6 +40,7 @@ # include "nwfilter_conf.h" # include "macvtap.h" # include "sysinfo.h" +# include "virobject.h" /* Private component of virDomainXMLFlags */ typedef enum { @@ -1144,8 +1145,8 @@ struct _virDomainDef { typedef struct _virDomainObj virDomainObj; typedef virDomainObj *virDomainObjPtr; struct _virDomainObj { + virObject obj; virMutex lock; - int refs; int pid; int state; @@ -1226,8 +1227,7 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, void virDomainDefFree(virDomainDefPtr vm); void virDomainObjRef(virDomainObjPtr vm); -/* Returns 1 if the object was freed, 0 if more refs exist */ -int virDomainObjUnref(virDomainObjPtr vm) ATTRIBUTE_RETURN_CHECK; +void virDomainObjUnref(virDomainObjPtr vm); /* live == true means def describes an active domain (being migrated or * restored) as opposed to a new persistent configuration of the domain */ diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 88cd4c8..c08ed3b 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -472,6 +472,11 @@ int openvzLoadDomains(struct openvz_driver *driver) { if (VIR_ALLOC(dom) < 0) goto no_memory; + if (virObjectInit(&dom->obj, NULL)) { + VIR_FREE(dom); + goto cleanup; + } + if (virMutexInit(&dom->lock) < 0) { openvzError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); @@ -489,7 +494,6 @@ int openvzLoadDomains(struct openvz_driver *driver) { else dom->state = VIR_DOMAIN_RUNNING; - dom->refs = 1; dom->pid = veid; dom->def->id = dom->state == VIR_DOMAIN_SHUTOFF ? -1 : veid; /* XXX OpenVZ doesn't appear to have concept of a transient domain */ @@ -554,7 +558,7 @@ int openvzLoadDomains(struct openvz_driver *driver) { VIR_FREE(outbuf); /* dom hasn't been shared yet, so unref should return 0 */ if (dom) - ignore_value(virDomainObjUnref(dom)); + virDomainObjUnref(dom); return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c2a1f9a..3a3c953 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -457,13 +457,13 @@ int qemuDomainObjBeginJob(virDomainObjPtr obj) } then = timeval_to_ms(now) + QEMU_JOB_WAIT_TIME; - virDomainObjRef(obj); + virDomainObjLock(obj); while (priv->jobActive) { if (virCondWaitUntil(&priv->jobCond, &obj->lock, then) < 0) { - /* Safe to ignore value since ref count was incremented above */ - ignore_value(virDomainObjUnref(obj)); - if (errno == ETIMEDOUT) + int err = errno; + virDomainObjUnlock(obj); + if (err == ETIMEDOUT) qemuReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", _("cannot acquire state change lock")); else @@ -482,12 +482,10 @@ int qemuDomainObjBeginJob(virDomainObjPtr obj) } /* - * obj must be locked before calling, qemud_driver must be locked - * * This must be called by anything that will change the VM state * in any way, or anything that will use the QEMU monitor. */ -int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, +int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver ATTRIBUTE_UNUSED, virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; @@ -501,20 +499,18 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, } then = timeval_to_ms(now) + QEMU_JOB_WAIT_TIME; - virDomainObjRef(obj); - qemuDriverUnlock(driver); + virDomainObjLock(obj); while (priv->jobActive) { if (virCondWaitUntil(&priv->jobCond, &obj->lock, then) < 0) { - /* Safe to ignore value since ref count was incremented above */ - ignore_value(virDomainObjUnref(obj)); - if (errno == ETIMEDOUT) + int err = errno; + virDomainObjUnlock(obj); + if (err == ETIMEDOUT) qemuReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", _("cannot acquire state change lock")); else virReportSystemError(errno, "%s", _("cannot acquire job mutex")); - qemuDriverLock(driver); return -1; } } @@ -524,10 +520,6 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, priv->jobStart = timeval_to_ms(now); memset(&priv->jobInfo, 0, sizeof(priv->jobInfo)); - virDomainObjUnlock(obj); - qemuDriverLock(driver); - virDomainObjLock(obj); - return 0; } @@ -540,7 +532,7 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, * Returns remaining refcount on 'obj', maybe 0 to indicated it * was deleted */ -int qemuDomainObjEndJob(virDomainObjPtr obj) +void qemuDomainObjEndJob(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; @@ -551,7 +543,7 @@ int qemuDomainObjEndJob(virDomainObjPtr obj) memset(&priv->jobInfo, 0, sizeof(priv->jobInfo)); virCondSignal(&priv->jobCond); - return virDomainObjUnref(obj); + virDomainObjUnlock(obj); } @@ -655,7 +647,7 @@ void qemuDomainObjExitRemoteWithDriver(struct qemud_driver *driver, virDomainObjLock(obj); /* Safe to ignore value, since we incremented ref in * qemuDomainObjEnterRemoteWithDriver */ - ignore_value(virDomainObjUnref(obj)); + virDomainObjUnref(obj); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8258900..12fd21d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -97,7 +97,7 @@ void qemuDomainSetNamespaceHooks(virCapsPtr caps); int qemuDomainObjBeginJob(virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; -int qemuDomainObjEndJob(virDomainObjPtr obj) ATTRIBUTE_RETURN_CHECK; +void qemuDomainObjEndJob(virDomainObjPtr obj); void qemuDomainObjEnterMonitor(virDomainObjPtr obj); void qemuDomainObjExitMonitor(virDomainObjPtr obj); void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..db89402 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -139,7 +139,6 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq struct qemuAutostartData *data = opaque; virErrorPtr err; - virDomainObjLock(vm); virResetLastError(); if (qemuDomainObjBeginJobWithDriver(data->driver, vm) < 0) { err = virGetLastError(); @@ -156,12 +155,8 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq err ? err->message : _("unknown error")); } - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); } - - if (vm) - virDomainObjUnlock(vm); } @@ -1055,12 +1050,13 @@ static virDomainPtr qemudDomainLookupByID(virConnectPtr conn, goto cleanup; } + virDomainObjLock(vm); dom = virGetDomain(conn, vm->def->name, vm->def->uuid); if (dom) dom->id = vm->def->id; + virDomainObjUnlock(vm); + virDomainObjUnref(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); return dom; } @@ -1082,12 +1078,13 @@ static virDomainPtr qemudDomainLookupByUUID(virConnectPtr conn, goto cleanup; } + virDomainObjLock(vm); dom = virGetDomain(conn, vm->def->name, vm->def->uuid); if (dom) dom->id = vm->def->id; + virDomainObjUnlock(vm); + virDomainObjUnref(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); return dom; } @@ -1107,12 +1104,13 @@ static virDomainPtr qemudDomainLookupByName(virConnectPtr conn, goto cleanup; } + virDomainObjLock(vm); dom = virGetDomain(conn, vm->def->name, vm->def->uuid); if (dom) dom->id = vm->def->id; + virDomainObjUnlock(vm); + virDomainObjUnref(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); return dom; } @@ -1133,11 +1131,13 @@ static int qemuDomainIsActive(virDomainPtr dom) _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } + + virDomainObjLock(obj); ret = virDomainObjIsActive(obj); + virDomainObjUnlock(obj); + virDomainObjUnref(obj); cleanup: - if (obj) - virDomainObjUnlock(obj); return ret; } @@ -1157,11 +1157,13 @@ static int qemuDomainIsPersistent(virDomainPtr dom) _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } + virDomainObjLock(obj); ret = obj->persistent; + virDomainObjUnlock(obj); cleanup: if (obj) - virDomainObjUnlock(obj); + virDomainObjUnref(obj); return ret; } @@ -1181,11 +1183,13 @@ static int qemuDomainIsUpdated(virDomainPtr dom) _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } + virDomainObjLock(obj); ret = obj->updated; + virDomainObjUnlock(obj); cleanup: if (obj) - virDomainObjUnlock(obj); + virDomainObjUnref(obj); return ret; } @@ -1239,39 +1243,57 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, qemuDriverLock(driver); if (!(def = virDomainDefParseString(driver->caps, xml, - VIR_DOMAIN_XML_INACTIVE))) + VIR_DOMAIN_XML_INACTIVE))) { + qemuDriverUnlock(driver); goto cleanup; + } - if (virSecurityManagerVerify(driver->securityManager, def) < 0) + if (virSecurityManagerVerify(driver->securityManager, def) < 0) { + qemuDriverUnlock(driver); goto cleanup; + } - if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) + if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) { + qemuDriverUnlock(driver); goto cleanup; + } - if (qemudCanonicalizeMachine(driver, def) < 0) + if (qemudCanonicalizeMachine(driver, def) < 0) { + qemuDriverUnlock(driver); goto cleanup; + } - if (qemuDomainAssignPCIAddresses(def) < 0) + if (qemuDomainAssignPCIAddresses(def) < 0) { + qemuDriverUnlock(driver); goto cleanup; + } if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, - def, false))) + def, false))) { + qemuDriverUnlock(driver); goto cleanup; + } + + qemuDriverUnlock(driver); def = NULL; - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) { + virDomainObjUnref(vm); goto cleanup; /* XXXX free the 'vm' we created ? */ + } if (qemuProcessStart(conn, driver, vm, NULL, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1, NULL, VIR_VM_OP_CREATE) < 0) { qemuAuditDomainStart(vm, "booted", false); - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, - vm); - vm = NULL; + qemuDomainObjEndJob(vm); + qemuDriverLock(driver); + virDomainRemoveInactive(&driver->domains, + vm); + qemuDriverUnlock(driver); + virDomainObjUnref(vm); goto cleanup; } @@ -1283,17 +1305,13 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, dom = virGetDomain(conn, vm->def->name, vm->def->uuid); if (dom) dom->id = vm->def->id; - if (vm && - qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); + virDomainObjUnref(vm); cleanup: virDomainDefFree(def); - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); return dom; } @@ -1307,13 +1325,14 @@ static int qemudDomainSuspend(virDomainPtr dom) { qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, @@ -1354,16 +1373,12 @@ static int qemudDomainSuspend(virDomainPtr dom) { } endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); - + virDomainObjUnref(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); return ret; } @@ -1376,6 +1391,7 @@ static int qemudDomainResume(virDomainPtr dom) { qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1409,15 +1425,12 @@ static int qemudDomainResume(virDomainPtr dom) { ret = 0; endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); + virDomainObjUnref(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); return ret; } @@ -1437,7 +1450,7 @@ static int qemudDomainShutdown(virDomainPtr dom) { virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } if (qemuDomainObjBeginJob(vm) < 0) @@ -1455,12 +1468,10 @@ static int qemudDomainShutdown(virDomainPtr dom) { qemuDomainObjExitMonitor(vm); endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -1473,12 +1484,13 @@ static int qemudDomainDestroy(virDomainPtr dom) { qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) @@ -1497,24 +1509,24 @@ static int qemudDomainDestroy(virDomainPtr dom) { qemuAuditDomainStop(vm, "destroyed"); if (!vm->persistent) { - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, - vm); + qemuDomainObjEndJob(vm); + virDomainObjUnref(vm); + qemuDriverLock(driver); + virDomainRemoveInactive(&driver->domains, vm); + qemuDriverUnlock(driver); vm = NULL; } ret = 0; endjob: - if (vm && - qemuDomainObjEndJob(vm) == 0) - vm = NULL; + if (vm) + qemuDomainObjEndJob(vm); cleanup: if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); return ret; } @@ -1535,12 +1547,14 @@ static char *qemudDomainGetOSType(virDomainPtr dom) { goto cleanup; } + virDomainObjLock(vm); if (!(type = strdup(vm->def->os.type))) virReportOOMError(); + virDomainObjUnlock(vm); cleanup: if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return type; } @@ -1562,11 +1576,13 @@ static unsigned long qemudDomainGetMaxMemory(virDomainPtr dom) { goto cleanup; } + virDomainObjLock(vm); ret = vm->def->mem.max_balloon; + virDomainObjUnlock(vm); cleanup: if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -1594,18 +1610,18 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } + if (qemuDomainObjBeginJob(vm) < 0) + goto cleanup; + if (newmem > vm->def->mem.max_balloon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("cannot set memory higher than max memory")); - goto cleanup; + goto endjob; } - if (qemuDomainObjBeginJob(vm) < 0) - goto cleanup; - if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_MEM_LIVE)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -1647,12 +1663,10 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, ret = 0; endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -1676,9 +1690,12 @@ static int qemudDomainGetInfo(virDomainPtr dom, virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } + if (qemuDomainObjBeginJob(vm) < 0) + goto cleanup; + info->state = vm->state; if (!virDomainObjIsActive(vm)) { @@ -1687,7 +1704,7 @@ static int qemudDomainGetInfo(virDomainPtr dom, if (qemudGetProcessInfo(&(info->cpuTime), NULL, vm->pid, 0) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot read cputime for domain")); - goto cleanup; + goto endjob; } } @@ -1700,8 +1717,6 @@ static int qemudDomainGetInfo(virDomainPtr dom, (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) { info->memory = vm->def->mem.max_balloon; } else if (!priv->jobActive) { - if (qemuDomainObjBeginJob(vm) < 0) - goto cleanup; if (!virDomainObjIsActive(vm)) err = 0; else { @@ -1709,10 +1724,6 @@ static int qemudDomainGetInfo(virDomainPtr dom, err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); qemuDomainObjExitMonitor(vm); } - if (qemuDomainObjEndJob(vm) == 0) { - vm = NULL; - goto cleanup; - } if (err < 0) goto cleanup; @@ -1731,9 +1742,10 @@ static int qemudDomainGetInfo(virDomainPtr dom, info->nrVirtCpu = vm->def->vcpus; ret = 0; +endjob: + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -2006,9 +2018,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); if (!vm->persistent) { - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, - vm); + qemuDomainObjEndJob(vm); + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } @@ -2021,8 +2032,8 @@ endjob: VIR_WARN0("Unable to resume guest CPUs after save failure"); } } - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + + qemuDomainObjEndJob(vm); } cleanup: @@ -2303,6 +2314,7 @@ static int qemudDomainCoreDump(virDomainPtr dom, qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); + qemuDriverUnlock(driver); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -2311,11 +2323,12 @@ static int qemudDomainCoreDump(virDomainPtr dom, _("no domain with matching uuid '%s'"), uuidstr); goto cleanup; } - priv = vm->privateData; if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) goto cleanup; + priv = vm->privateData; + if (!virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -2367,20 +2380,20 @@ endjob: } } - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; - else if ((ret == 0) && (flags & VIR_DUMP_CRASH) && !vm->persistent) { + qemuDomainObjEndJob(vm); + if ((ret == 0) && (flags & VIR_DUMP_CRASH) && !vm->persistent) { + qemuDriverLock(driver); virDomainRemoveInactive(&driver->domains, vm); + qemuDriverUnlock(driver); vm = NULL; } cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); + if (vm) + virDomainObjUnref(vm); return ret; } @@ -2403,9 +2416,6 @@ static void processWatchdogEvent(void *data, void *opaque) break; } - qemuDriverLock(driver); - virDomainObjLock(wdEvent->vm); - if (qemuDomainObjBeginJobWithDriver(driver, wdEvent->vm) < 0) break; @@ -2429,10 +2439,7 @@ static void processWatchdogEvent(void *data, void *opaque) qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Resuming after dump failed")); - if (qemuDomainObjEndJob(wdEvent->vm) > 0) - virDomainObjUnlock(wdEvent->vm); - - qemuDriverUnlock(driver); + qemuDomainObjEndJob(wdEvent->vm); VIR_FREE(dumpfile); } @@ -2533,7 +2540,7 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } if (qemuDomainObjBeginJob(vm) < 0) @@ -2608,12 +2615,10 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, ret = virDomainSaveConfig(driver->configDir, persistentDef); endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -2645,7 +2650,7 @@ qemudDomainPinVcpu(virDomainPtr dom, virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } if (!virDomainObjIsActive(vm)) { @@ -2690,8 +2695,7 @@ qemudDomainPinVcpu(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -2717,7 +2721,7 @@ qemudDomainGetVcpus(virDomainPtr dom, virUUIDFormat(dom->uuid, uuidstr); qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; + return -1; } if (!virDomainObjIsActive(vm)) { @@ -2780,8 +2784,7 @@ qemudDomainGetVcpus(virDomainPtr dom, ret = maxinfo; cleanup: - if (vm) - virDomainObjUnlock(vm); + virDomainObjUnref(vm); return ret; } @@ -3164,9 +3167,8 @@ qemuDomainRestore(virConnectPtr conn, ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; - else if (ret < 0 && !vm->persistent) { + qemuDomainObjEndJob(vm); + if (ret < 0 && !vm->persistent) { virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } @@ -3174,8 +3176,6 @@ qemuDomainRestore(virConnectPtr conn, cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -3254,10 +3254,7 @@ static char *qemudDomainDumpXML(virDomainPtr dom, qemuDomainObjEnterMonitorWithDriver(driver, vm); err = qemuMonitorGetBalloonInfo(priv->mon, &balloon); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (qemuDomainObjEndJob(vm) == 0) { - vm = NULL; - goto cleanup; - } + qemuDomainObjEndJob(vm); if (err < 0) goto cleanup; if (err > 0) @@ -3269,8 +3266,6 @@ static char *qemudDomainDumpXML(virDomainPtr dom, ret = qemuDomainFormatXML(driver, vm, flags); cleanup: - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -3487,12 +3482,9 @@ qemudDomainStartWithFlags(virDomainPtr dom, unsigned int flags) (flags & VIR_DOMAIN_START_PAUSED) != 0); endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -3859,8 +3851,7 @@ static int qemudDomainAttachDevice(virDomainPtr dom, ret = -1; endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: if (cgroup) @@ -3868,8 +3859,6 @@ cleanup: qemuCapsFree(qemuCaps); virDomainDeviceDefFree(dev); - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -3993,8 +3982,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, ret = -1; endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: if (cgroup) @@ -4002,8 +3990,6 @@ cleanup: qemuCapsFree(qemuCaps); virDomainDeviceDefFree(dev); - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -4083,14 +4069,11 @@ static int qemudDomainDetachDevice(virDomainPtr dom, ret = -1; endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: qemuCapsFree(qemuCaps); virDomainDeviceDefFree(dev); - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -4802,12 +4785,9 @@ qemudDomainBlockStats (virDomainPtr dom, qemuDomainObjExitMonitor(vm); endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); return ret; } @@ -4906,12 +4886,9 @@ qemudDomainMemoryStats (virDomainPtr dom, "%s", _("domain is not running")); } - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); return ret; } @@ -5064,15 +5041,12 @@ qemudDomainMemoryPeek (virDomainPtr dom, ret = 0; endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: VIR_FORCE_CLOSE(fd); unlink (tmp); VIR_FREE(tmp); - if (vm) - virDomainObjUnlock(vm); return ret; } @@ -5218,16 +5192,13 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, qemuDomainObjExitMonitor(vm); } - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); } else { ret = 0; } cleanup: VIR_FORCE_CLOSE(fd); - if (vm) - virDomainObjUnlock(vm); return ret; } @@ -6067,8 +6038,8 @@ cleanup: _("resuming after snapshot failed")); } - if (qemuDomainObjEndJob(vm) == 0) - *vmptr = NULL; + qemuDomainObjEndJob(vm); + *vmptr = NULL; return ret; } @@ -6438,9 +6409,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, vm); - vm = NULL; + qemuDomainObjEndJob(vm); + virDomainRemoveInactive(&driver->domains, vm); goto cleanup; } } @@ -6454,14 +6424,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, ret = 0; endjob: - if (vm && qemuDomainObjEndJob(vm) == 0) - vm = NULL; + if (vm) + qemuDomainObjEndJob(vm); cleanup: if (event) qemuDomainEventQueue(driver, event); - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; @@ -6675,12 +6643,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, ret = qemuDomainSnapshotDiscard(driver, vm, snap); endjob: - if (qemuDomainObjEndJob(vm) == 0) - vm = NULL; + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } @@ -6727,14 +6692,9 @@ static int qemuDomainMonitorCommand(virDomainPtr domain, const char *cmd, qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorArbitraryCommand(priv->mon, cmd, result, hmp); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (qemuDomainObjEndJob(vm) == 0) { - vm = NULL; - goto cleanup; - } + qemuDomainObjEndJob(vm); cleanup: - if (vm) - virDomainObjUnlock(vm); qemuDriverUnlock(driver); return ret; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 43741e1..462e6be 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -336,8 +336,8 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, qemuAuditDomainStart(vm, "migrated", false); qemuProcessStop(driver, vm, 0); if (!vm->persistent) { - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, vm); + qemuDomainObjEndJob(vm); + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } virReportSystemError(errno, "%s", @@ -353,9 +353,10 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, ret = 0; endjob: - if (vm && - qemuDomainObjEndJob(vm) == 0) + if (vm) { + qemuDomainObjEndJob(vm); vm = NULL; + } /* We set a fake job active which is held across * API calls until the finish() call. This prevents @@ -374,11 +375,8 @@ cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); - qemuDriverUnlock(driver); return ret; } @@ -530,8 +528,8 @@ qemuMigrationPrepareDirect(struct qemud_driver *driver, * should have already done that. */ if (!vm->persistent) { - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, vm); + qemuDomainObjEndJob(vm); + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } goto endjob; @@ -544,9 +542,10 @@ qemuMigrationPrepareDirect(struct qemud_driver *driver, ret = 0; endjob: - if (vm && - qemuDomainObjEndJob(vm) == 0) + if (vm) { + qemuDomainObjEndJob(vm); vm = NULL; + } /* We set a fake job active which is held across * API calls until the finish() call. This prevents @@ -565,8 +564,6 @@ cleanup: virDomainDefFree(def); if (ret != 0) VIR_FREE(*uri_out); - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); return ret; @@ -1090,8 +1087,8 @@ int qemuMigrationPerform(struct qemud_driver *driver, VIR_DOMAIN_EVENT_STOPPED_MIGRATED); if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) { virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm); - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, vm); + qemuDomainObjEndJob(vm); + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } ret = 0; @@ -1112,13 +1109,12 @@ endjob: VIR_DOMAIN_EVENT_RESUMED, VIR_DOMAIN_EVENT_RESUMED_MIGRATED); } - if (vm && - qemuDomainObjEndJob(vm) == 0) + if (vm) { + qemuDomainObjEndJob(vm); vm = NULL; + } cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); return ret; @@ -1266,20 +1262,19 @@ qemuMigrationFinish(struct qemud_driver *driver, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); if (!vm->persistent) { - if (qemuDomainObjEndJob(vm) > 0) - virDomainRemoveInactive(&driver->domains, vm); + qemuDomainObjEndJob(vm); + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; } } endjob: - if (vm && - qemuDomainObjEndJob(vm) == 0) + if (vm) { + qemuDomainObjEndJob(vm); vm = NULL; + } cleanup: - if (vm) - virDomainObjUnlock(vm); if (event) qemuDomainEventQueue(driver, event); return dom; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 48ecd5c..244b22a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -604,8 +604,8 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon, priv = vm->privateData; if (priv->mon == mon) priv->mon = NULL; - if (virDomainObjUnref(vm) > 0) - virDomainObjUnlock(vm); + virDomainObjUnlock(vm); + virDomainObjUnref(vm); } static qemuMonitorCallbacks monitorCallbacks = { @@ -644,7 +644,7 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm) /* Safe to ignore value since ref count was incremented above */ if (priv->mon == NULL) - ignore_value(virDomainObjUnref(vm)); + virDomainObjUnref(vm); if (virSecurityManagerClearSocketLabel(driver->securityManager, vm) < 0) { VIR_ERROR(_("Failed to clear security context for monitor for %s"), @@ -1940,10 +1940,6 @@ qemuProcessReconnect(void *payload, const void *name ATTRIBUTE_UNUSED, void *opa priv = obj->privateData; - /* Hold an extra reference because we can't allow 'vm' to be - * deleted if qemuConnectMonitor() failed */ - virDomainObjRef(obj); - /* XXX check PID liveliness & EXE path */ if (qemuConnectMonitor(driver, obj) < 0) goto error; @@ -1975,8 +1971,7 @@ qemuProcessReconnect(void *payload, const void *name ATTRIBUTE_UNUSED, void *opa if (obj->def->id >= driver->nextvmid) driver->nextvmid = obj->def->id + 1; - if (virDomainObjUnref(obj) > 0) - virDomainObjUnlock(obj); + virDomainObjUnlock(obj); qemuCapsFree(qemuCaps); return; @@ -1984,21 +1979,17 @@ qemuProcessReconnect(void *payload, const void *name ATTRIBUTE_UNUSED, void *opa error: qemuCapsFree(qemuCaps); if (!virDomainObjIsActive(obj)) { - if (virDomainObjUnref(obj) > 0) - virDomainObjUnlock(obj); + virDomainObjUnlock(obj); return; } - if (virDomainObjUnref(obj) > 0) { - /* We can't get the monitor back, so must kill the VM - * to remove danger of it ending up running twice if - * user tries to start it again later */ - qemuProcessStop(driver, obj, 0); - if (!obj->persistent) - virDomainRemoveInactive(&driver->domains, obj); - else - virDomainObjUnlock(obj); - } + /* We can't get the monitor back, so must kill the VM + * to remove danger of it ending up running twice if + * user tries to start it again later */ + qemuProcessStop(driver, obj, 0); + virDomainObjUnlock(obj); + if (!obj->persistent) + virDomainRemoveInactive(&driver->domains, obj); } /** diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index 6339248..6b59b18 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -205,7 +205,7 @@ cleanup: VIR_FREE(vmx); /* any non-NULL vm here has not been shared, so unref will return 0 */ if (vm) - ignore_value(virDomainObjUnref(vm)); + virDomainObjUnref(vm); return ret; } -- 1.7.3.1 -- Thanks, Hu Tao

On Wed, Apr 06, 2011 at 04:33:56PM -0600, Eric Blake wrote: How to use virReportErrorHelper in this case? Sorry for my bad English, I don't understand this sentence. Agreed. But this needs many changes I think it is better to do it in a seperate patch. Would it be better to have two types of hashtable, one hashes only virObjects, the other hashes data except virObjects? this can minimize the impact brought by the change of hashtable to existing code. OK. Don't understand you very well here. Let me explain what I understand: If a dom is added into a hashtable, then it is not safe to modify its uuid since it is the key of dom in hashtable. And we have to make some rules about when the uuid can be modified(say, hold the driver lock first when dom is in hashtable). If I understand correctly, then I think the simple way to modify the uuid is to remove the dom from hashtable, modify uuid, then reinsert it into hashtable. The qemu driver lock here is to protect the hashtable objs(remove entry), not for unref dom. However, I think this code should be better like this: qemu driver lock dom = virHashRemoveEntry(); /* not be exactly but a way to get the removed dom */ qemu driver unlock unref dom OK. Will check this carefully. Yes, you are right. OK, makes code cleaner. Yes, not safe here. Since the semantics of qemuDomainObjBeginJobWithDriver is changed, and many places call it, I have to check every place and test the change. In next version I plan to split the series into patches like this: first a patch changes virDomainObj to inherit from virObject, second a patch changes qemuDomainObjBeginJobWithDriver, then each patch for a place that calls qemuDomainObjBeginJobWithDriver. Is this reasonable? Or a better way? As you have already noticed, the semantics of qemuDomainObjBeginJobWithDriver and qemuEnterMonitorWithDriver(patch 4) are changed, qemu driver lock have not to be hold before calling them. So if this patch is applied, patch 4 should also be applied. Will remove it. Will remove it.

At 04/12/2011 01:51 PM, Hu Tao Write: We lock qemu_driver and vm as the folling steps: 1. lock qemu_driver 2. lock vm We try to lock qemu_driver while holding the vm's lock. OOps, it will cause the libvirtd deadlock.(I can reproduce this bug by your script) We have unref it, so do not unref it again.

On Tue, Apr 12, 2011 at 06:33:21PM +0800, Hu Tao wrote: ACK 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 :|

On Wed, Apr 06, 2011 at 03:19:51PM +0800, Hu Tao wrote: This is a major change in semantics, which makes pretty much every single caller non-thread safe, unless the callers are all changed todo virDomainObjLock immediately after calling this. So I don't really see the point in this - it just means more code duplication. Simiarly here, you're now requiring all callers to manually obtain a lock. driver is now unlocked.... ...but this method *requires* driver to be locked. ...and this method writes to 'driver', so it is now unsafe. And all the usage of 'vm' in this method is now completely unlocked and unsafe. Also now completely unsafe since 'vm' is never locked while making changes to it. [cut rest of patch] I don't see how any of this patch is threadsafe now that virtually no methods are acquiring the 'vm' lock. 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 :|

--- src/qemu/qemu_domain.c | 30 ++----------- src/qemu/qemu_migration.c | 2 - src/qemu/qemu_monitor.c | 109 ++++++++++++++++++++++++-------------------- src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_process.c | 32 +++++++------- 5 files changed, 81 insertions(+), 96 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3a3c953..d11dc5f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -560,9 +560,8 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; - qemuMonitorLock(priv->mon); - qemuMonitorRef(priv->mon); virDomainObjUnlock(obj); + qemuMonitorLock(priv->mon); } @@ -573,18 +572,9 @@ void qemuDomainObjEnterMonitor(virDomainObjPtr obj) void qemuDomainObjExitMonitor(virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; - int refs; - - refs = qemuMonitorUnref(priv->mon); - - if (refs > 0) - qemuMonitorUnlock(priv->mon); + qemuMonitorUnlock(priv->mon); virDomainObjLock(obj); - - if (refs == 0) { - priv->mon = NULL; - } } @@ -601,10 +591,8 @@ void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, { qemuDomainObjPrivatePtr priv = obj->privateData; - qemuMonitorLock(priv->mon); - qemuMonitorRef(priv->mon); virDomainObjUnlock(obj); - qemuDriverUnlock(driver); + qemuMonitorLock(priv->mon); } @@ -617,19 +605,9 @@ void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj) { qemuDomainObjPrivatePtr priv = obj->privateData; - int refs; - - refs = qemuMonitorUnref(priv->mon); - - if (refs > 0) - qemuMonitorUnlock(priv->mon); - qemuDriverLock(driver); + qemuMonitorUnlock(priv->mon); virDomainObjLock(obj); - - if (refs == 0) { - priv->mon = NULL; - } } void qemuDomainObjEnterRemoteWithDriver(struct qemud_driver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 462e6be..6af2e24 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -224,11 +224,9 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm) } virDomainObjUnlock(vm); - qemuDriverUnlock(driver); nanosleep(&ts, NULL); - qemuDriverLock(driver); virDomainObjLock(vm); } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2d28f8d..98c55e7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -37,6 +37,7 @@ #include "memory.h" #include "logging.h" #include "files.h" +#include "virobject.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -44,11 +45,10 @@ #define DEBUG_RAW_IO 0 struct _qemuMonitor { + virObject obj; virMutex lock; /* also used to protect fd */ virCond notify; - int refs; - int fd; int watch; int hasSendFD; @@ -203,35 +203,61 @@ void qemuMonitorUnlock(qemuMonitorPtr mon) } -static void qemuMonitorFree(qemuMonitorPtr mon) +static void doMonitorFree(virObjectPtr obj) { + qemuMonitorPtr mon = (qemuMonitorPtr)obj; + VIR_DEBUG("mon=%p", mon); if (mon->cb && mon->cb->destroy) (mon->cb->destroy)(mon, mon->vm); - if (virCondDestroy(&mon->notify) < 0) - {} + ignore_value(virCondDestroy(&mon->notify)); virMutexDestroy(&mon->lock); VIR_FREE(mon->buffer); VIR_FREE(mon); } -int qemuMonitorRef(qemuMonitorPtr mon) +static qemuMonitorPtr qemuMonitorAlloc(void) { - mon->refs++; - return mon->refs; -} + qemuMonitorPtr mon; -int qemuMonitorUnref(qemuMonitorPtr mon) -{ - mon->refs--; + if (VIR_ALLOC(mon) < 0) { + virReportOOMError(); + return NULL; + } - if (mon->refs == 0) { - qemuMonitorUnlock(mon); - qemuMonitorFree(mon); - return 0; + if (virObjectInit(&mon->obj, doMonitorFree)) { + VIR_FREE(mon); + return NULL; + } + + if (virMutexInit(&mon->lock) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize monitor mutex")); + VIR_FREE(mon); + return NULL; } - return mon->refs; + if (virCondInit(&mon->notify) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize monitor condition")); + virMutexDestroy(&mon->lock); + VIR_FREE(mon); + return NULL; + } + + mon->fd = -1; + + return mon; +} + +void qemuMonitorRef(qemuMonitorPtr mon) +{ + virObjectRef(&mon->obj); +} + +void qemuMonitorUnref(qemuMonitorPtr mon) +{ + virObjectUnref(&mon->obj); } static void @@ -239,9 +265,7 @@ qemuMonitorUnwatch(void *monitor) { qemuMonitorPtr mon = monitor; - qemuMonitorLock(mon); - if (qemuMonitorUnref(mon) > 0) - qemuMonitorUnlock(mon); + qemuMonitorUnref(mon); } static int @@ -521,7 +545,6 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { /* lock access to the monitor and protect fd */ qemuMonitorLock(mon); - qemuMonitorRef(mon); #if DEBUG_IO VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, fd, events); #endif @@ -598,13 +621,11 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) { /* Make sure anyone waiting wakes up now */ virCondSignal(&mon->notify); - if (qemuMonitorUnref(mon) > 0) - qemuMonitorUnlock(mon); + qemuMonitorUnlock(mon); VIR_DEBUG("Triggering EOF callback error? %d", failed); (eofNotify)(mon, vm, failed); } else { - if (qemuMonitorUnref(mon) > 0) - qemuMonitorUnlock(mon); + qemuMonitorUnlock(mon); } } @@ -623,30 +644,13 @@ qemuMonitorOpen(virDomainObjPtr vm, return NULL; } - if (VIR_ALLOC(mon) < 0) { - virReportOOMError(); + mon = qemuMonitorAlloc(); + if (!mon) return NULL; - } - if (virMutexInit(&mon->lock) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize monitor mutex")); - VIR_FREE(mon); - return NULL; - } - if (virCondInit(&mon->notify) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot initialize monitor condition")); - virMutexDestroy(&mon->lock); - VIR_FREE(mon); - return NULL; - } - mon->fd = -1; - mon->refs = 1; mon->vm = vm; mon->json = json; mon->cb = cb; - qemuMonitorLock(mon); switch (config->type) { case VIR_DOMAIN_CHR_TYPE_UNIX: @@ -679,20 +683,25 @@ qemuMonitorOpen(virDomainObjPtr vm, } + /* mon will be accessed by qemuMonitorIO which is called in + * event thread, so ref it before passing it to the thread. + * + * Note: mon is unrefed in qemuMonitorUnwatch + */ + qemuMonitorRef(mon); if ((mon->watch = virEventAddHandle(mon->fd, VIR_EVENT_HANDLE_HANGUP | VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_READABLE, qemuMonitorIO, mon, qemuMonitorUnwatch)) < 0) { + qemuMonitorUnref(mon); qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unable to register monitor events")); goto cleanup; } - qemuMonitorRef(mon); VIR_DEBUG("New mon %p fd =%d watch=%d", mon, mon->fd, mon->watch); - qemuMonitorUnlock(mon); return mon; @@ -703,8 +712,8 @@ cleanup: * so kill the callbacks now. */ mon->cb = NULL; - qemuMonitorUnlock(mon); qemuMonitorClose(mon); + qemuMonitorUnref(mon); return NULL; } @@ -724,8 +733,8 @@ void qemuMonitorClose(qemuMonitorPtr mon) VIR_FORCE_CLOSE(mon->fd); } - if (qemuMonitorUnref(mon) > 0) - qemuMonitorUnlock(mon); + qemuMonitorUnlock(mon); + qemuMonitorUnref(mon); } @@ -778,7 +787,7 @@ int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, if ((mon)->cb && (mon)->cb->callback) \ (ret) = ((mon)->cb->callback)(mon, __VA_ARGS__); \ qemuMonitorLock(mon); \ - ignore_value(qemuMonitorUnref(mon)); \ + qemuMonitorUnref(mon); \ } while (0) int qemuMonitorGetDiskSecret(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c90219b..4498c49 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -132,8 +132,8 @@ int qemuMonitorCheckHMP(qemuMonitorPtr mon, const char *cmd); void qemuMonitorLock(qemuMonitorPtr mon); void qemuMonitorUnlock(qemuMonitorPtr mon); -int qemuMonitorRef(qemuMonitorPtr mon); -int qemuMonitorUnref(qemuMonitorPtr mon) ATTRIBUTE_RETURN_CHECK; +void qemuMonitorRef(qemuMonitorPtr mon); +void qemuMonitorUnref(qemuMonitorPtr mon); /* These APIs are for use by the internal Text/JSON monitor impl code only */ int qemuMonitorSend(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 244b22a..4b9087f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -107,7 +107,6 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, VIR_DEBUG("Received EOF on %p '%s'", vm, vm->def->name); - qemuDriverLock(driver); virDomainObjLock(vm); if (!virDomainObjIsActive(vm)) { @@ -133,15 +132,17 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuProcessStop(driver, vm, 0); qemuAuditDomainStop(vm, hasError ? "failed" : "shutdown"); - if (!vm->persistent) + if (!vm->persistent) { + qemuDriverLock(driver); virDomainRemoveInactive(&driver->domains, vm); - else - virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + } + + virDomainObjUnlock(vm); if (event) { qemuDomainEventQueue(driver, event); } - qemuDriverUnlock(driver); } @@ -602,8 +603,10 @@ static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon, virDomainObjLock(vm); priv = vm->privateData; - if (priv->mon == mon) + if (priv->mon == mon) { priv->mon = NULL; + qemuMonitorUnref(mon); + } virDomainObjUnlock(vm); virDomainObjUnref(vm); } @@ -633,19 +636,11 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm) goto error; } - /* Hold an extra reference because we can't allow 'vm' to be - * deleted while the monitor is active */ - virDomainObjRef(vm); - priv->mon = qemuMonitorOpen(vm, priv->monConfig, priv->monJSON, &monitorCallbacks); - /* Safe to ignore value since ref count was incremented above */ - if (priv->mon == NULL) - virDomainObjUnref(vm); - if (virSecurityManagerClearSocketLabel(driver->securityManager, vm) < 0) { VIR_ERROR(_("Failed to clear security context for monitor for %s"), vm->def->name); @@ -657,6 +652,7 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr vm) goto error; } + qemuMonitorRef(priv->mon); qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorSetCapabilities(priv->mon); @@ -2471,8 +2467,12 @@ void qemuProcessStop(struct qemud_driver *driver, _("Failed to send SIGTERM to %s (%d)"), vm->def->name, vm->pid); - if (priv->mon) - qemuMonitorClose(priv->mon); + if (priv->mon) { + qemuMonitorPtr mon = priv->mon; + priv->mon = NULL; + qemuMonitorClose(mon); + qemuMonitorUnref(mon); + } if (priv->monConfig) { if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) -- 1.7.3.1 -- Thanks, Hu Tao

On Thu, Apr 07, 2011 at 10:38:34AM +0100, Daniel P. Berrange wrote: qemuDomainObjEnterMonitorWithDriver is now called without holding qemu driver lock. Now qemuMigrationWaitForCompletion should be called without holding qemu driver lock. Excepting for introducing virObject for reference-counting, this series also simplifies the usage of lock: if you want to read/write qemu driver data, it is enough to first acquire qemu driver lock only; if you want to read/write virDomainObj data, it is enough to first acquire virDomainObj lock only; same for others. And we'd better to avoid acquiring two locks at the same time. So yes, the code here is problematic, it should be ideally like this: virDomainObjLock(vm); if (!vm->persistent) { lock_hashtable(doms); /* hashtable's own lock to protect itself */ virDomainRemoveInactive(doms, vm); unlock_hashtable(doms); } virDomainObjUnlock(vm); But it lacks hashtable lock, how about change the code like this: virDomainObjLock(vm); persistent = vm->persistent; virDomainObjUnlock(vm); /* chances that others change vm->persistent and we remove vm mistakenly :( */ if (!persistent) { qemuDriverLock(driver); virDomainRemoveInactive(doms, vm); qemuDriverUnlock(driver); } Or is there a better way?

On Fri, Apr 08, 2011 at 09:57:16AM +0100, Daniel P. Berrange wrote: Sorry I didn't make this clear, will do in next version.