On 07/31/2012 10:58 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
Given recent changes, this now fails to apply cleanly. Would you mind
refreshing the series, to make it easier to review?
> 16 files changed, 448 insertions(+), 1086 deletions(-)
Love the diffstat!
I still spotted some nits, even going solely by code inspection:
> @@ -225,64 +217,21 @@ 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)
> +{
> -
> - if (conn) {
> - VIR_DEBUG("unref connection %p %d", conn, conn->refs);
> - conn->refs--;
> - if (conn->refs == 0) {
> - virReleaseConnect(conn);
> - /* Already unlocked mutex */
> - return;
> - }
> - virMutexUnlock(&conn->lock);
> - }
> + if (domain->conn)
> + virObjectUnref(domain->conn);
Technically, we have a bug if we ever have domain->conn==NULL when we
get here. Besides, you coded virObjectUnref(NULL) to be a graceful
no-op, so that means you should add virObjectUnref to cfg.mk's list of
free-like functions and drop the conditional.
Oh, that reminds me - we DO have a bug in our RPC code where we fail to
check for NULL after strdup and such in the various make_nonnull_*
functions, and therefore I suppose that is an instance where we really
_could_ end up with domain->conn being NULL at the moment - I've been
meaning to spend some time on fixing that.
> @@ -349,60 +289,17 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
> * which may also be released if its ref count hits zero.
> */
> static void
> -virReleaseNetwork(virNetworkPtr network) {
> - virConnectPtr conn = network->conn;
> +virNetworkDispose(void *obj)
> +{
> + virNetworkPtr network = obj;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> + if (network->conn)
> + virObjectUnref(network->conn);
Another place where you can drop the conditional.
> @@ -484,58 +366,15 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) {
> * which may also be released if its ref count hits zero.
> */
> static void
> -virReleaseInterface(virInterfacePtr iface) {
> - virConnectPtr conn = iface->conn;
> +virInterfaceDispose(void *obj)
> +{
> + virInterfacePtr iface = obj;
> + if (iface->conn)
> + virObjectUnref(iface->conn);
and again
> @@ -608,60 +438,17 @@ error:
> * which may also be released if its ref count hits zero.
> */
> static void
> -virReleaseStoragePool(virStoragePoolPtr pool) {
> - virConnectPtr conn = pool->conn;
> +virStoragePoolDispose(void *obj)
> +{
> + virStoragePoolPtr pool = obj;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> + if (pool->conn)
> + virObjectUnref(pool->conn);
I'm turning into a broken record...
> @@ -748,59 +516,16 @@ error:
> * which may also be released if its ref count hits zero.
> */
> static void
> -virReleaseStorageVol(virStorageVolPtr vol) {
> - virConnectPtr conn = vol->conn;
> +virStorageVolDispose(void *obj)
> +{
> + virStorageVolPtr vol = obj;
> + if (vol->conn)
> + virObjectUnref(vol->conn);
Need I say it? :)
I'll quit mentioning it (again, a cfg.mk rule will make it easier to
find all instances).
> +++ b/src/datatypes.h
> +# define VIR_IS_CONNECT(obj) \
> + (virObjectIsClass((virObjectPtr)(obj), virConnectClass))
Why the extra cast here? The compiler can convert obj to 'void*'
without your help.
> +
> +# define VIR_IS_DOMAIN(obj) \
> + (virObjectIsClass((virObjectPtr)(obj), virDomainClass))
Likewise throughout these macros.
> +++ b/src/libvirt.c
> @@ -1420,14 +1420,16 @@ error:
> * matching virConnectClose, and all other references will be released
> * after the corresponding operation completes.
> *
> - * Returns the number of remaining references on success
> - * (positive implies that some other call still has a reference open,
> - * 0 implies that no references remain and the connection is closed),
> - * or -1 on failure. It is possible for the last virConnectClose to
> - * return a positive value if some other object still has a temporary
> - * reference to the connection, but the application should not try to
> - * further use a connection after the virConnectClose that matches the
> - * initial open.
> + * Returns the a positive number if at least 1 reference remains on
s/the a/a/
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> +virSecretClass;
> +virStorageVolClass;
> +virStoragePoolClass;
Sorting.
> +++ b/src/xen/xend_internal.c
> @@ -1237,7 +1237,7 @@ error:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("failed to parse Xend domain information"));
> if (ret != NULL)
> - virUnrefDomain(ret);
> + virObjectUnref(ret);
Another useless if before free.
> +++ b/tests/sexpr2xmltest.c
> @@ -72,7 +72,7 @@ testCompareFiles(const char *xml, const char *sexpr, int xendConfigVersion)
> VIR_FREE(gotxml);
> virDomainDefFree(def);
> if (conn)
> - virUnrefConnect(conn);
> + virObjectUnref(conn);
Oops, I promised not to point them out :)
--
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