On 07/11/2012 07:35 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list