6:08 p.m.
On 07/11/2012 07:35 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
This converts the following public API datatypes to use the
virObject infrastructure:
virConnectPtr
virDomainPtr
virDomainSnapshotPtr
virInterfacePtr
virNetworkPtr
virNodeDevicePtr
virNWFilterPtr
virSecretPtr
virStreamPtr
virStorageVolPtr
virStoragePoolPtr
The code is significantly simplified, since the mutex in the
virConnectPtr object now only needs to be held when accessing
the per-connection virError object instance. All other operations
are completely lock free.
Nice!
* src/datatypes.c, src/datatypes.h, src/libvirt.c: Convert
public datatypes to use virObject
* src/conf/domain_event.c, src/phyp/phyp_driver.c,
src/qemu/qemu_command.c, src/qemu/qemu_migration.c,
src/qemu/qemu_process.c, src/storage/storage_driver.c,
src/vbox/vbox_tmpl.c, src/xen/xend_internal.c,
tests/qemuxml2argvtest.c, tests/qemuxmlnstest.c,
tests/sexpr2xmltest.c, tests/xmconfigtest.c: Convert
to use virObjectUnref/virObjectRef
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
src/conf/domain_event.c | 8 +-
src/datatypes.c | 999 ++++++++++--------------------------------
src/datatypes.h | 264 ++++-------
src/libvirt.c | 127 ++----
src/libvirt_private.syms | 17 +-
src/phyp/phyp_driver.c | 14 +-
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_migration.c | 8 +-
src/qemu/qemu_process.c | 2 +-
src/storage/storage_driver.c | 4 +-
src/vbox/vbox_tmpl.c | 2 +-
src/xen/xend_internal.c | 4 +-
tests/qemuxml2argvtest.c | 21 +-
tests/qemuxmlnstest.c | 2 +-
tests/sexpr2xmltest.c | 2 +-
tests/xmconfigtest.c | 4 +-
16 files changed, 410 insertions(+), 1070 deletions(-)
Love the diffstat. However, it means I didn't get completely through
this review today; I'll have to pick up on the review on Monday.
+static int virDataTypesOnceInit(void)
+{
+#define DECLARE_CLASS(basename) \
+ if (!(basename ## Class = virClassNew(#basename, \
+ sizeof(basename), \
+ basename ## Dispose))) \
+ return -1;
+
+ DECLARE_CLASS(virConnect);
+ DECLARE_CLASS(virDomain);
+ DECLARE_CLASS(virDomainSnapshot);
+ DECLARE_CLASS(virInterface);
+ DECLARE_CLASS(virNetwork);
+ DECLARE_CLASS(virNodeDevice);
+ DECLARE_CLASS(virNWFilter);
+ DECLARE_CLASS(virSecret);
+ DECLARE_CLASS(virStream);
+ DECLARE_CLASS(virStorageVol);
+ DECLARE_CLASS(virStoragePool);
+
+ return 0;
#undef DECLARE_CLASS
to make the macro scope match the function scope where it was used.
+}
+
+VIR_ONCE_GLOBAL_INIT(virDataTypes)
Do we need to tweak 1/13 to allow a trailing ';' here? The only reason
to want a ';' would be if emacs and/or vi auto-indentation would be
helped by having it.
/**
* virGetConnect:
@@ -53,35 +96,28 @@ virConnectPtr
virGetConnect(void) {
virConnectPtr ret;
- if (VIR_ALLOC(ret) < 0) {
- virReportOOMError();
- goto failed;
- }
+ if (virDataTypesInitialize() < 0)
+ return NULL;
+
+ if (!(ret = virObjectNew(virConnectClass)))
+ return NULL;
+
if (virMutexInit(&ret->lock) < 0) {
VIR_FREE(ret);
- goto failed;
+ return NULL;
}
- ret->magic = VIR_CONNECT_MAGIC;
ret->driver = NULL;
ret->networkDriver = NULL;
ret->privateData = NULL;
ret->networkPrivateData = NULL;
ret->interfacePrivateData = NULL;
Aren't all these assignments to NULL dead code, given that
virObjectNew() (and VIR_ALLOC() before it) guarantee zero-initialization
of a new object?
@@ -89,14 +125,8 @@ failed:
* be released prior to this returning. The connection obj must not
* be used once this method returns.
*/
-static void
-virReleaseConnect(virConnectPtr conn) {
- VIR_DEBUG("release connection %p", conn);
-
- /* make sure to release the connection lock before we call the
- * close callbacks, otherwise we will deadlock if an error
- * is raised by any of the callbacks */
- virMutexUnlock(&conn->lock);
+static void virConnectDispose(void *obj) {
Style nit - I think our code base has tended towards the layout of:
static void
virConnectDispose(void *obj)
{
rather than your one-line squash, as it makes it easier to find starts
of functions (emacs in particular has key-presses that assume my layout
and get lost on your layout).
virDomainPtr
virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
virDomainPtr ret = NULL;
- char uuidstr[VIR_UUID_STRING_BUFLEN];
Oh my - we were writing but never reading this string. Nice cleanup of
a wasted operation. But it might be worth an independent commit, since
it wasn't mentioned in your commit message. Or was the intent to keep
this code, and log not only the name but also the pretty-printed UUID of
each newly-created domain object?
@@ -225,62 +212,20 @@ virGetDomain(virConnectPtr conn, const char
*name, const unsigned char *uuid) {
* which may also be released if its ref count hits zero.
*/
static void
-virReleaseDomain(virDomainPtr domain) {
- virConnectPtr conn = domain->conn;
+virDomainDispose(void *obj) {
This' one's better (two lines instead of one), but still not the
preferred style of three lines.
@@ -290,14 +235,16 @@ virUnrefDomain(virDomainPtr domain) {
* Lookup if the network is already registered for that connection,
* if yes return a new pointer to it, if no allocate a new structure,
* and register it in the table. In any case a corresponding call to
- * virUnrefNetwork() is needed to not leak data.
+ * virObjectUnref() is needed to not leak data.
*
* Returns a pointer to the network, or NULL in case of failure
*/
virNetworkPtr
virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
virNetworkPtr ret = NULL;
- char uuidstr[VIR_UUID_STRING_BUFLEN];
Another cleanup of wasted computation (or a missing pretty-printed log
message).
+ ret->conn = virObjectRef(conn);
This can set ret->conn to NULL if conn was NULL on input; do we need to
check for that? Or is it only possible on a user bug (which we should
have already filtered out in libvirt.c when checking that we had a valid
connection pointer in the first place), and/or due to our missing OOM
handling in the RPC code (which I've been meaning to cleanup ever since
I noticed it on the virDomainListAll code)?
@@ -426,6 +316,9 @@ virInterfacePtr
virGetInterface(virConnectPtr conn, const char *name, const char *mac) {
And that's as far as I got.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org