Converting from virObject to GObject is reasonably straightforward,
as illustrated by this patch for virIdentity

In the header file

 - Remove

     typedef struct _virIdentity virIdentity

 - Add

     #define VIR_TYPE_IDENTITY virIdentity_get_type ()
     G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject);

   Which provides the typedef we just removed, and class
   declaration boilerplate and various other constants/macros.

In the source file

 - Change 'virObject parent' to 'GObject parent' in the struct
 - Remove the virClass variable and its initializing call
 - Add

      G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT)

   which declares the instance & class constructor functions

 - Add an impl of the instance & class constructors
   wiring up the finalize method to point to our dispose impl

In all files

 - Replace VIR_AUTOUNREF(virIdentityPtr) with g_autoptr(virIdentity)

 - Replace virObjectRef/Unref with g_object_ref/unref. Note
   the latter functions do *NOT* accept a NULL object where as
   libvirt's do. If you replace g_object_unref with g_clear_object
   it is NULL safe, but also clears the pointer.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/access/viraccessdriverpolkit.c  | 21 +++----
 src/admin/admin_server.c            |  3 +-
 src/qemu/qemu_process.c             |  4 +-
 src/remote/remote_daemon.c          |  3 +-
 src/remote/remote_daemon_dispatch.c | 35 ++++--------
 src/rpc/virnetserverclient.c        | 57 ++++++++-----------
 src/rpc/virnetserverprogram.c       | 13 +----
 src/util/viridentity.c              | 87 ++++++++++++++++-------------
 src/util/viridentity.h              |  7 ++-
 tests/viridentitytest.c             | 45 ++++++---------
 tests/virnetserverclienttest.c      |  3 +-
 11 files changed, 122 insertions(+), 156 deletions(-)

diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c
index e61ac6fa19..b39a28c834 100644
--- a/src/access/viraccessdriverpolkit.c
+++ b/src/access/viraccessdriverpolkit.c
@@ -78,8 +78,7 @@ virAccessDriverPolkitGetCaller(const char *actionid,
                                unsigned long long *startTime,
                                uid_t *uid)
 {
-    virIdentityPtr identity = virIdentityGetCurrent();
-    int ret = -1;
+    g_autoptr(virIdentity) identity = virIdentityGetCurrent();
     int rc;
 
     if (!identity) {
@@ -90,37 +89,33 @@ virAccessDriverPolkitGetCaller(const char *actionid,
     }
 
     if ((rc = virIdentityGetProcessID(identity, pid)) < 0)
-        goto cleanup;
+        return -1;
 
     if (rc == 0) {
         virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("No process ID available"));
-        goto cleanup;
+        return -1;
     }
 
     if ((rc = virIdentityGetProcessTime(identity, startTime)) < 0)
-        goto cleanup;
+        return -1;
 
     if (rc == 0) {
         virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("No process start time available"));
-        goto cleanup;
+        return -1;
     }
 
     if ((rc = virIdentityGetUNIXUserID(identity, uid)) < 0)
-        goto cleanup;
+        return -1;
 
     if (rc == 0) {
         virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("No UNIX caller UID available"));
-        goto cleanup;
+        return -1;
     }
 
-    ret = 0;
-
- cleanup:
-    virObjectUnref(identity);
-    return ret;
+    return 0;
 }
 
 
diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
index 248df3f795..7ae819b7b0 100644
--- a/src/admin/admin_server.c
+++ b/src/admin/admin_server.c
@@ -221,7 +221,7 @@ adminClientGetInfo(virNetServerClientPtr client,
     char *sock_addr = NULL;
     const char *attr = NULL;
     virTypedParameterPtr tmpparams = NULL;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     int rc;
 
     virCheckFlags(0, -1);
@@ -310,7 +310,6 @@ adminClientGetInfo(virNetServerClientPtr client,
     ret = 0;
 
  cleanup:
-    virObjectUnref(identity);
     VIR_FREE(sock_addr);
     return ret;
 }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a50cd54393..872750ff4d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8051,7 +8051,7 @@ qemuProcessReconnect(void *opaque)
     bool tryMonReconn = false;
 
     virIdentitySetCurrent(data->identity);
-    virObjectUnref(data->identity);
+    g_clear_object(&data->identity);
     VIR_FREE(data);
 
     qemuDomainObjRestoreJob(obj, &oldjob);
@@ -8364,7 +8364,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
 
         virDomainObjEndAPI(&obj);
         virNWFilterUnlockFilterUpdates();
-        virObjectUnref(data->identity);
+        g_clear_object(&data->identity);
         VIR_FREE(data);
         return -1;
     }
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 9281e226c4..7fcaa31c49 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -826,7 +826,7 @@ handleSystemMessageFunc(DBusConnection *connection ATTRIBUTE_UNUSED,
 static void daemonRunStateInit(void *opaque)
 {
     virNetDaemonPtr dmn = opaque;
-    virIdentityPtr sysident = virIdentityGetSystem();
+    g_autoptr(virIdentity) sysident = virIdentityGetSystem();
 #ifdef MODULE_NAME
     bool mandatory = true;
 #else /* ! MODULE_NAME */
@@ -879,7 +879,6 @@ static void daemonRunStateInit(void *opaque)
  cleanup:
     daemonInhibitCallback(false, dmn);
     virObjectUnref(dmn);
-    virObjectUnref(sysident);
     virIdentitySetCurrent(NULL);
 }
 
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index dbd2985c38..9fc5dc3998 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -159,7 +159,7 @@ remoteRelayDomainEventCheckACL(virNetServerClientPtr client,
                                virConnectPtr conn, virDomainPtr dom)
 {
     virDomainDef def;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     bool ret = false;
 
     /* For now, we just create a virDomainDef with enough contents to
@@ -177,7 +177,6 @@ remoteRelayDomainEventCheckACL(virNetServerClientPtr client,
 
  cleanup:
     ignore_value(virIdentitySetCurrent(NULL));
-    virObjectUnref(identity);
     return ret;
 }
 
@@ -187,7 +186,7 @@ remoteRelayNetworkEventCheckACL(virNetServerClientPtr client,
                                 virConnectPtr conn, virNetworkPtr net)
 {
     virNetworkDef def;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     bool ret = false;
 
     /* For now, we just create a virNetworkDef with enough contents to
@@ -204,7 +203,6 @@ remoteRelayNetworkEventCheckACL(virNetServerClientPtr client,
 
  cleanup:
     ignore_value(virIdentitySetCurrent(NULL));
-    virObjectUnref(identity);
     return ret;
 }
 
@@ -214,7 +212,7 @@ remoteRelayStoragePoolEventCheckACL(virNetServerClientPtr client,
                                     virStoragePoolPtr pool)
 {
     virStoragePoolDef def;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     bool ret = false;
 
     /* For now, we just create a virStoragePoolDef with enough contents to
@@ -231,7 +229,6 @@ remoteRelayStoragePoolEventCheckACL(virNetServerClientPtr client,
 
  cleanup:
     ignore_value(virIdentitySetCurrent(NULL));
-    virObjectUnref(identity);
     return ret;
 }
 
@@ -241,7 +238,7 @@ remoteRelayNodeDeviceEventCheckACL(virNetServerClientPtr client,
                                    virNodeDevicePtr dev)
 {
     virNodeDeviceDef def;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     bool ret = false;
 
     /* For now, we just create a virNodeDeviceDef with enough contents to
@@ -257,7 +254,6 @@ remoteRelayNodeDeviceEventCheckACL(virNetServerClientPtr client,
 
  cleanup:
     ignore_value(virIdentitySetCurrent(NULL));
-    virObjectUnref(identity);
     return ret;
 }
 
@@ -267,7 +263,7 @@ remoteRelaySecretEventCheckACL(virNetServerClientPtr client,
                                virSecretPtr secret)
 {
     virSecretDef def;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     bool ret = false;
 
     /* For now, we just create a virSecretDef with enough contents to
@@ -285,7 +281,6 @@ remoteRelaySecretEventCheckACL(virNetServerClientPtr client,
 
  cleanup:
     ignore_value(virIdentitySetCurrent(NULL));
-    virObjectUnref(identity);
     return ret;
 }
 
@@ -294,7 +289,7 @@ remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClientPtr client,
                                           virConnectPtr conn, virDomainPtr dom)
 {
     virDomainDef def;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
     bool ret = false;
 
     /* For now, we just create a virDomainDef with enough contents to
@@ -311,7 +306,6 @@ remoteRelayDomainQemuMonitorEventCheckACL(virNetServerClientPtr client,
 
  cleanup:
     ignore_value(virIdentitySetCurrent(NULL));
-    virObjectUnref(identity);
     return ret;
 }
 
@@ -1869,7 +1863,7 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int r
 static void
 remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv)
 {
-    virIdentityPtr sysident = virIdentityGetSystem();
+    g_autoptr(virIdentity) sysident = virIdentityGetSystem();
     virIdentitySetCurrent(sysident);
 
     DEREG_CB(priv->conn, priv->domainEventCallbacks,
@@ -1898,7 +1892,6 @@ remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv)
     }
 
     virIdentitySetCurrent(NULL);
-    virObjectUnref(sysident);
 }
 #undef DEREG_CB
 
@@ -1965,7 +1958,7 @@ remoteOpenConn(const char *uri,
     }
 
     if (preserveIdentity) {
-        VIR_AUTOUNREF(virIdentityPtr) ident = NULL;
+        g_autoptr(virIdentity) ident = NULL;
 
         if (!(ident = virIdentityGetCurrent()))
             return -1;
@@ -2436,7 +2429,7 @@ remoteDispatchConnectSetIdentity(virNetServerPtr server ATTRIBUTE_UNUSED,
     int nparams = 0;
     int rv = -1;
     virConnectPtr conn = remoteGetHypervisorConn(client);
-    VIR_AUTOUNREF(virIdentityPtr) ident = NULL;
+    g_autoptr(virIdentity) ident = NULL;
     if (!conn)
         goto cleanup;
 
@@ -3982,7 +3975,7 @@ static int
 remoteSASLFinish(virNetServerPtr server,
                  virNetServerClientPtr client)
 {
-    virIdentityPtr clnt_identity = NULL;
+    g_autoptr(virIdentity) clnt_identity = NULL;
     const char *identity;
     struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
     int ssf;
@@ -3990,7 +3983,7 @@ remoteSASLFinish(virNetServerPtr server,
     /* TLS or UNIX domain sockets trivially OK */
     if (!virNetServerClientIsSecure(client)) {
         if ((ssf = virNetSASLSessionGetKeySize(priv->sasl)) < 0)
-            goto error;
+            return -1;
 
         VIR_DEBUG("negotiated an SSF of %d", ssf);
         if (ssf < 56) { /* 56 is good for Kerberos */
@@ -4006,7 +3999,7 @@ remoteSASLFinish(virNetServerPtr server,
         return -2;
 
     if (!(clnt_identity = virNetServerClientGetIdentity(client)))
-        goto error;
+        return -1;
 
     virNetServerSetClientAuthenticated(server, client);
     virNetServerClientSetSASLSession(client, priv->sasl);
@@ -4018,14 +4011,10 @@ remoteSASLFinish(virNetServerPtr server,
           "client=%p auth=%d identity=%s",
           client, REMOTE_AUTH_SASL, identity);
 
-    virObjectUnref(clnt_identity);
     virObjectUnref(priv->sasl);
     priv->sasl = NULL;
 
     return 0;
-
- error:
-    return -1;
 }
 
 /*
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 171ee636dd..8482c5c29c 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -759,13 +759,13 @@ int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client,
 static virIdentityPtr
 virNetServerClientCreateIdentity(virNetServerClientPtr client)
 {
-    char *username = NULL;
-    char *groupname = NULL;
-    char *seccontext = NULL;
-    virIdentityPtr ret = NULL;
+    g_autofree char *username = NULL;
+    g_autofree char *groupname = NULL;
+    g_autofree char *seccontext = NULL;
+    g_autoptr(virIdentity) ret = NULL;
 
     if (!(ret = virIdentityNew()))
-        goto error;
+        return NULL;
 
     if (client->sock && virNetSocketIsLocal(client->sock)) {
         gid_t gid;
@@ -775,59 +775,50 @@ virNetServerClientCreateIdentity(virNetServerClientPtr client)
         if (virNetSocketGetUNIXIdentity(client->sock,
                                         &uid, &gid, &pid,
                                         &timestamp) < 0)
-            goto error;
+            return NULL;
 
         if (!(username = virGetUserName(uid)))
-            goto error;
+            return NULL;
         if (virIdentitySetUserName(ret, username) < 0)
-            goto error;
+            return NULL;
         if (virIdentitySetUNIXUserID(ret, uid) < 0)
-            goto error;
+            return NULL;
 
         if (!(groupname = virGetGroupName(gid)))
-            goto error;
+            return NULL;
         if (virIdentitySetGroupName(ret, groupname) < 0)
-            goto error;
+            return NULL;
         if (virIdentitySetUNIXGroupID(ret, gid) < 0)
-            goto error;
+            return NULL;
 
         if (virIdentitySetProcessID(ret, pid) < 0)
-            goto error;
+            return NULL;
         if (virIdentitySetProcessTime(ret, timestamp) < 0)
-            goto error;
+            return NULL;
     }
 
 #if WITH_SASL
     if (client->sasl) {
         const char *identity = virNetSASLSessionGetIdentity(client->sasl);
         if (virIdentitySetSASLUserName(ret, identity) < 0)
-            goto error;
+            return NULL;
     }
 #endif
 
     if (client->tls) {
         const char *identity = virNetTLSSessionGetX509DName(client->tls);
         if (virIdentitySetX509DName(ret, identity) < 0)
-            goto error;
+            return NULL;
     }
 
     if (client->sock &&
         virNetSocketGetSELinuxContext(client->sock, &seccontext) < 0)
-        goto error;
+        return NULL;
     if (seccontext &&
         virIdentitySetSELinuxContext(ret, seccontext) < 0)
-        goto error;
-
- cleanup:
-    VIR_FREE(username);
-    VIR_FREE(groupname);
-    VIR_FREE(seccontext);
-    return ret;
+        return NULL;
 
- error:
-    virObjectUnref(ret);
-    ret = NULL;
-    goto cleanup;
+    return g_steal_pointer(&ret);
 }
 
 
@@ -838,7 +829,7 @@ virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client)
     if (!client->identity)
         client->identity = virNetServerClientCreateIdentity(client);
     if (client->identity)
-        ret = virObjectRef(client->identity);
+        ret = g_object_ref(client->identity);
     virObjectUnlock(client);
     return ret;
 }
@@ -848,10 +839,10 @@ void virNetServerClientSetIdentity(virNetServerClientPtr client,
                                    virIdentityPtr identity)
 {
     virObjectLock(client);
-    virObjectUnref(client->identity);
+    g_clear_object(&client->identity);
     client->identity = identity;
     if (client->identity)
-        virObjectRef(client->identity);
+        g_object_ref(client->identity);
     virObjectUnlock(client);
 }
 
@@ -988,7 +979,7 @@ void virNetServerClientDispose(void *obj)
     if (client->privateData)
         client->privateDataFreeFunc(client->privateData);
 
-    virObjectUnref(client->identity);
+    g_clear_object(&client->identity);
 
 #if WITH_SASL
     virObjectUnref(client->sasl);
@@ -1683,7 +1674,7 @@ virNetServerClientGetInfo(virNetServerClientPtr client,
         goto cleanup;
     }
 
-    *identity = virObjectRef(client->identity);
+    *identity = g_object_ref(client->identity);
 
     ret = 0;
  cleanup:
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c
index 7ae1d2e955..cca820ca5a 100644
--- a/src/rpc/virnetserverprogram.c
+++ b/src/rpc/virnetserverprogram.c
@@ -370,13 +370,13 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog,
                                 virNetServerClientPtr client,
                                 virNetMessagePtr msg)
 {
-    char *arg = NULL;
-    char *ret = NULL;
+    g_autofree char *arg = NULL;
+    g_autofree char *ret = NULL;
     int rv = -1;
     virNetServerProgramProcPtr dispatcher;
     virNetMessageError rerr;
     size_t i;
-    virIdentityPtr identity = NULL;
+    g_autoptr(virIdentity) identity = NULL;
 
     memset(&rerr, 0, sizeof(rerr));
 
@@ -484,10 +484,7 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog,
     }
 
     xdr_free(dispatcher->ret_filter, ret);
-    VIR_FREE(arg);
-    VIR_FREE(ret);
 
-    virObjectUnref(identity);
     /* Put reply on end of tx queue to send out  */
     return virNetServerClientSendMessage(client, msg);
 
@@ -496,10 +493,6 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog,
      * RPC error message we can send back to the client */
     rv = virNetServerProgramSendReplyError(prog, client, msg, &rerr, &msg->header);
 
-    VIR_FREE(arg);
-    VIR_FREE(ret);
-    virObjectUnref(identity);
-
     return rv;
 }
 
diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 22e2644c19..467d953e17 100644
--- a/src/util/viridentity.c
+++ b/src/util/viridentity.c
@@ -43,25 +43,29 @@
 VIR_LOG_INIT("util.identity");
 
 struct _virIdentity {
-    virObject parent;
+    GObject parent;
 
     int nparams;
     int maxparams;
     virTypedParameterPtr params;
 };
 
-static virClassPtr virIdentityClass;
+G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT)
+
 static virThreadLocal virIdentityCurrent;
 
-static void virIdentityDispose(void *obj);
+static void virIdentityDispose(GObject *obj);
 
-static int virIdentityOnceInit(void)
+static void virIdentityCurrentCleanup(void *ident)
 {
-    if (!VIR_CLASS_NEW(virIdentity, virClassForObject()))
-        return -1;
+    if (ident)
+        g_object_unref(ident);
+}
 
+static int virIdentityOnceInit(void)
+{
     if (virThreadLocalInit(&virIdentityCurrent,
-                           (virThreadLocalCleanup)virObjectUnref) < 0) {
+                           virIdentityCurrentCleanup) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Cannot initialize thread local for current identity"));
         return -1;
@@ -72,13 +76,24 @@ static int virIdentityOnceInit(void)
 
 VIR_ONCE_GLOBAL_INIT(virIdentity);
 
+static void virIdentity_init(virIdentity *ident G_GNUC_UNUSED)
+{
+}
+
+static void virIdentity_class_init(virIdentityClass *klass)
+{
+    GObjectClass *obj = G_OBJECT_CLASS(klass);
+
+    obj->finalize = virIdentityDispose;
+}
+
 /**
  * virIdentityGetCurrent:
  *
  * Get the current identity associated with this thread. The
  * caller will own a reference to the returned identity, but
  * must not modify the object in any way, other than to
- * release the reference when done with virObjectUnref
+ * release the reference when done with g_object_unref
  *
  * Returns: a reference to the current identity, or NULL
  */
@@ -90,7 +105,9 @@ virIdentityPtr virIdentityGetCurrent(void)
         return NULL;
 
     ident = virThreadLocalGet(&virIdentityCurrent);
-    return virObjectRef(ident);
+    if (ident)
+        g_object_ref(ident);
+    return ident;
 }
 
 
@@ -105,7 +122,7 @@ virIdentityPtr virIdentityGetCurrent(void)
  */
 int virIdentitySetCurrent(virIdentityPtr ident)
 {
-    virIdentityPtr old;
+    g_autoptr(virIdentity) old = NULL;
 
     if (virIdentityInitialize() < 0)
         return -1;
@@ -113,15 +130,14 @@ int virIdentitySetCurrent(virIdentityPtr ident)
     old = virThreadLocalGet(&virIdentityCurrent);
 
     if (virThreadLocalSet(&virIdentityCurrent,
-                          virObjectRef(ident)) < 0) {
+                          ident ? g_object_ref(ident) : NULL) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Unable to set thread local identity"));
-        virObjectUnref(ident);
+        if (ident)
+            g_object_unref(ident);
         return -1;
     }
 
-    virObjectUnref(old);
-
     return 0;
 }
 
@@ -139,36 +155,36 @@ virIdentityPtr virIdentityGetSystem(void)
     VIR_AUTOFREE(char *) username = NULL;
     VIR_AUTOFREE(char *) groupname = NULL;
     unsigned long long startTime;
-    virIdentityPtr ret = NULL;
+    g_autoptr(virIdentity) ret = NULL;
 #if WITH_SELINUX
     security_context_t con;
 #endif
 
     if (!(ret = virIdentityNew()))
-        goto error;
+        return NULL;
 
     if (virIdentitySetProcessID(ret, getpid()) < 0)
-        goto error;
+        return NULL;
 
     if (virProcessGetStartTime(getpid(), &startTime) < 0)
-        goto error;
+        return NULL;
     if (startTime != 0 &&
         virIdentitySetProcessTime(ret, startTime) < 0)
-        goto error;
+        return NULL;
 
     if (!(username = virGetUserName(geteuid())))
         return ret;
     if (virIdentitySetUserName(ret, username) < 0)
-        goto error;
+        return NULL;
     if (virIdentitySetUNIXUserID(ret, getuid()) < 0)
-        goto error;
+        return NULL;
 
     if (!(groupname = virGetGroupName(getegid())))
         return ret;
     if (virIdentitySetGroupName(ret, groupname) < 0)
-        goto error;
+        return NULL;
     if (virIdentitySetUNIXGroupID(ret, getgid()) < 0)
-        goto error;
+        return NULL;
 
 #if WITH_SELINUX
     if (is_selinux_enabled() > 0) {
@@ -179,17 +195,13 @@ virIdentityPtr virIdentityGetSystem(void)
         }
         if (virIdentitySetSELinuxContext(ret, con) < 0) {
             freecon(con);
-            goto error;
+            return NULL;
         }
         freecon(con);
     }
 #endif
 
-    return ret;
-
- error:
-    virObjectUnref(ret);
-    return NULL;
+    return g_steal_pointer(&ret);
 }
 
 
@@ -203,26 +215,21 @@ virIdentityPtr virIdentityGetSystem(void)
  */
 virIdentityPtr virIdentityNew(void)
 {
-    virIdentityPtr ident;
-
-    if (virIdentityInitialize() < 0)
-        return NULL;
-
-    if (!(ident = virObjectNew(virIdentityClass)))
-        return NULL;
-
-    return ident;
+    return VIR_IDENTITY(g_object_new(VIR_TYPE_IDENTITY, NULL));
 }
 
 
-static void virIdentityDispose(void *object)
+static void virIdentityDispose(GObject *object)
 {
-    virIdentityPtr ident = object;
+    virIdentityPtr ident = VIR_IDENTITY(object);
 
     virTypedParamsFree(ident->params, ident->nparams);
+
+    G_OBJECT_CLASS(virIdentity_parent_class)->finalize(object);
 }
 
 
+
 /*
  * Returns: 0 if not present, 1 if present, -1 on error
  */
diff --git a/src/util/viridentity.h b/src/util/viridentity.h
index 861ecca736..b63a53e6c4 100644
--- a/src/util/viridentity.h
+++ b/src/util/viridentity.h
@@ -21,9 +21,12 @@
 
 #pragma once
 
-#include "virobject.h"
+#include "internal.h"
+#include <glib-object.h>
+
+#define VIR_TYPE_IDENTITY virIdentity_get_type ()
+G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject);
 
-typedef struct _virIdentity virIdentity;
 typedef virIdentity *virIdentityPtr;
 
 virIdentityPtr virIdentityGetCurrent(void);
diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c
index 1eadd6173a..81db049fcb 100644
--- a/tests/viridentitytest.c
+++ b/tests/viridentitytest.c
@@ -38,58 +38,52 @@ VIR_LOG_INIT("tests.identitytest");
 
 static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED)
 {
-    int ret = -1;
-    virIdentityPtr ident;
+    g_autoptr(virIdentity) ident = NULL;
     const char *val;
     int rc;
 
-    if (!(ident = virIdentityNew()))
-        goto cleanup;
+    ident = virIdentityNew();
 
     if (virIdentitySetUserName(ident, "fred") < 0)
-        goto cleanup;
+        return -1;
 
     if ((rc = virIdentityGetUserName(ident, &val)) < 0)
-        goto cleanup;
+        return -1;
 
     if (STRNEQ_NULLABLE(val, "fred") || rc != 1) {
         VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val));
-        goto cleanup;
+        return -1;
     }
 
     if ((rc = virIdentityGetGroupName(ident, &val)) < 0)
-        goto cleanup;
+        return -1;
 
     if (val != NULL || rc != 0) {
         VIR_DEBUG("Unexpected groupname attribute");
-        goto cleanup;
+        return -1;
     }
 
     if (virIdentitySetUserName(ident, "joe") >= 0) {
         VIR_DEBUG("Unexpectedly overwrote attribute");
-        goto cleanup;
+        return -1;
     }
 
     if ((rc = virIdentityGetUserName(ident, &val)) < 0)
-        goto cleanup;
+        return -1;
 
     if (STRNEQ_NULLABLE(val, "fred") || rc != 1) {
         VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val));
-        goto cleanup;
+        return -1;
     }
 
-    ret = 0;
- cleanup:
-    virObjectUnref(ident);
-    return ret;
+    return 0;
 }
 
 
 static int testIdentityGetSystem(const void *data)
 {
     const char *context = data;
-    int ret = -1;
-    virIdentityPtr ident = NULL;
+    g_autoptr(virIdentity) ident = NULL;
     const char *val;
     int rc;
 
@@ -97,35 +91,32 @@ static int testIdentityGetSystem(const void *data)
     if (context) {
         VIR_DEBUG("libvirt not compiled with SELinux, skipping this test");
         ret = EXIT_AM_SKIP;
-        goto cleanup;
+        return -1;
     }
 #endif
 
     if (!(ident = virIdentityGetSystem())) {
         VIR_DEBUG("Unable to get system identity");
-        goto cleanup;
+        return -1;
     }
 
     if ((rc = virIdentityGetSELinuxContext(ident, &val)) < 0)
-        goto cleanup;
+        return -1;
 
     if (context == NULL) {
         if (val != NULL || rc != 0) {
             VIR_DEBUG("Unexpected SELinux context %s", NULLSTR(val));
-            goto cleanup;
+            return -1;
         }
     } else {
         if (STRNEQ_NULLABLE(val, context) || rc != 1) {
             VIR_DEBUG("Want SELinux context '%s' got '%s'",
                       context, val);
-            goto cleanup;
+            return -1;
         }
     }
 
-    ret = 0;
- cleanup:
-    virObjectUnref(ident);
-    return ret;
+    return 0;
 }
 
 static int testSetFakeSELinuxContext(const void *data ATTRIBUTE_UNUSED)
diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c
index d094de9840..42393d7dbe 100644
--- a/tests/virnetserverclienttest.c
+++ b/tests/virnetserverclienttest.c
@@ -51,7 +51,7 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
     int ret = -1;
     virNetSocketPtr sock = NULL;
     virNetServerClientPtr client = NULL;
-    virIdentityPtr ident = NULL;
+    g_autoptr(virIdentity) ident = NULL;
     const char *gotUsername = NULL;
     uid_t gotUserID;
     const char *gotGroupname = NULL;
@@ -141,7 +141,6 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
     if (client)
         virNetServerClientClose(client);
     virObjectUnref(client);
-    virObjectUnref(ident);
     VIR_FORCE_CLOSE(sv[0]);
     VIR_FORCE_CLOSE(sv[1]);
     return ret;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 27, 2019 at 06:17:29PM +0100, Daniel P. Berrangé wrote:
>Converting from virObject to GObject is reasonably straightforward,
>as illustrated by this patch for virIdentity
>

The change would be much easier to see if this patch did not contain
the g_autofree changes and the removal of cleanup/error labels, which
generate a lot of churn.

>In the header file
>
> - Remove
>
>     typedef struct _virIdentity virIdentity
>
> - Add
>
>     #define VIR_TYPE_IDENTITY virIdentity_get_type ()
>     G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject);
>
>   Which provides the typedef we just removed, and class
>   declaration boilerplate and various other constants/macros.
>
>In the source file
>
> - Change 'virObject parent' to 'GObject parent' in the struct
> - Remove the virClass variable and its initializing call
> - Add
>
>      G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT)
>
>   which declares the instance & class constructor functions
>
> - Add an impl of the instance & class constructors
>   wiring up the finalize method to point to our dispose impl
>
>In all files
>
> - Replace VIR_AUTOUNREF(virIdentityPtr) with g_autoptr(virIdentity)
>

Is the idea to never mix VIR_ALLOC/VIR_AUTOFREE with g_alloc/g_auto*?
If not, this step could be separated by temporarily setting
virObjectUnref as the cleanup function for g_autoptr.

Jano

> - Replace virObjectRef/Unref with g_object_ref/unref. Note
>   the latter functions do *NOT* accept a NULL object where as
>   libvirt's do. If you replace g_object_unref with g_clear_object
>   it is NULL safe, but also clears the pointer.
>
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> src/access/viraccessdriverpolkit.c  | 21 +++----
> src/admin/admin_server.c            |  3 +-
> src/qemu/qemu_process.c             |  4 +-
> src/remote/remote_daemon.c          |  3 +-
> src/remote/remote_daemon_dispatch.c | 35 ++++--------
> src/rpc/virnetserverclient.c        | 57 ++++++++-----------
> src/rpc/virnetserverprogram.c       | 13 +----
> src/util/viridentity.c              | 87 ++++++++++++++++-------------
> src/util/viridentity.h              |  7 ++-
> tests/viridentitytest.c             | 45 ++++++---------
> tests/virnetserverclienttest.c      |  3 +-
> 11 files changed, 122 insertions(+), 156 deletions(-)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 30, 2019 at 03:04:38PM +0200, Ján Tomko wrote:
> On Fri, Sep 27, 2019 at 06:17:29PM +0100, Daniel P. Berrangé wrote:
> > Converting from virObject to GObject is reasonably straightforward,
> > as illustrated by this patch for virIdentity
> > 
> 
> The change would be much easier to see if this patch did not contain
> the g_autofree changes and the removal of cleanup/error labels, which
> generate a lot of churn.
> 
> > In the header file
> > 
> > - Remove
> > 
> >     typedef struct _virIdentity virIdentity
> > 
> > - Add
> > 
> >     #define VIR_TYPE_IDENTITY virIdentity_get_type ()
> >     G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject);
> > 
> >   Which provides the typedef we just removed, and class
> >   declaration boilerplate and various other constants/macros.
> > 
> > In the source file
> > 
> > - Change 'virObject parent' to 'GObject parent' in the struct
> > - Remove the virClass variable and its initializing call
> > - Add
> > 
> >      G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT)
> > 
> >   which declares the instance & class constructor functions
> > 
> > - Add an impl of the instance & class constructors
> >   wiring up the finalize method to point to our dispose impl
> > 
> > In all files
> > 
> > - Replace VIR_AUTOUNREF(virIdentityPtr) with g_autoptr(virIdentity)
> > 
> 
> Is the idea to never mix VIR_ALLOC/VIR_AUTOFREE with g_alloc/g_auto*?

Functionally there shouldn't be any problem with mixing the cleanup
/ allocator APIs..

> If not, this step could be separated by temporarily setting
> virObjectUnref as the cleanup function for g_autoptr.

I'll have more of a think about separating the changes.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 27, 2019 at 06:17:29PM +0100, Daniel P. Berrangé wrote:
> Converting from virObject to GObject is reasonably straightforward,
> as illustrated by this patch for virIdentity
> 
> In the header file
> 
>  - Remove
> 
>      typedef struct _virIdentity virIdentity
> 
>  - Add
> 
>      #define VIR_TYPE_IDENTITY virIdentity_get_type ()
>      G_DECLARE_FINAL_TYPE (virIdentity, virIdentity, VIR, IDENTITY, GObject);
> 
>    Which provides the typedef we just removed, and class
>    declaration boilerplate and various other constants/macros.
> 
> In the source file
> 
>  - Change 'virObject parent' to 'GObject parent' in the struct
>  - Remove the virClass variable and its initializing call
>  - Add
> 
>       G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT)
> 
>    which declares the instance & class constructor functions
> 
>  - Add an impl of the instance & class constructors
>    wiring up the finalize method to point to our dispose impl
> 
> In all files
> 
>  - Replace VIR_AUTOUNREF(virIdentityPtr) with g_autoptr(virIdentity)
> 
>  - Replace virObjectRef/Unref with g_object_ref/unref. Note
>    the latter functions do *NOT* accept a NULL object where as
>    libvirt's do. If you replace g_object_unref with g_clear_object
>    it is NULL safe, but also clears the pointer.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/access/viraccessdriverpolkit.c  | 21 +++----
>  src/admin/admin_server.c            |  3 +-
>  src/qemu/qemu_process.c             |  4 +-
>  src/remote/remote_daemon.c          |  3 +-
>  src/remote/remote_daemon_dispatch.c | 35 ++++--------
>  src/rpc/virnetserverclient.c        | 57 ++++++++-----------
>  src/rpc/virnetserverprogram.c       | 13 +----
>  src/util/viridentity.c              | 87 ++++++++++++++++-------------
>  src/util/viridentity.h              |  7 ++-
>  tests/viridentitytest.c             | 45 ++++++---------
>  tests/virnetserverclienttest.c      |  3 +-
>  11 files changed, 122 insertions(+), 156 deletions(-)

As Jan pointed out this patch should do the minimal conversion to
GObject to demonstrate the transition.  Let's move the cleanup into
separate patch.

I'm OK with using g_autoptr for the new GObject as we don't have to
define anything else for it to work and that's what we want to
eventually use in our code base.

> diff --git a/src/util/viridentity.c b/src/util/viridentity.c
> index 22e2644c19..467d953e17 100644
> --- a/src/util/viridentity.c
> +++ b/src/util/viridentity.c
> @@ -43,25 +43,29 @@
>  VIR_LOG_INIT("util.identity");
>  
>  struct _virIdentity {
> -    virObject parent;
> +    GObject parent;
>  
>      int nparams;
>      int maxparams;
>      virTypedParameterPtr params;
>  };
>  
> -static virClassPtr virIdentityClass;
> +G_DEFINE_TYPE(virIdentity, virIdentity, G_TYPE_OBJECT)
> +
>  static virThreadLocal virIdentityCurrent;
>  
> -static void virIdentityDispose(void *obj);
> +static void virIdentityDispose(GObject *obj);

We should rename it to virIdentityFinalize as there is a difference
between finalize and dispose in glib.

Dispose may be called multiple times and should only remove and clear
references to other objects for cyclic dependencies, finalize is called
only once and should actually free any resources.

> -static int virIdentityOnceInit(void)
> +static void virIdentityCurrentCleanup(void *ident)
>  {
> -    if (!VIR_CLASS_NEW(virIdentity, virClassForObject()))
> -        return -1;
> +    if (ident)
> +        g_object_unref(ident);
> +}
>  
> +static int virIdentityOnceInit(void)
> +{
>      if (virThreadLocalInit(&virIdentityCurrent,
> -                           (virThreadLocalCleanup)virObjectUnref) < 0) {
> +                           virIdentityCurrentCleanup) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Cannot initialize thread local for current identity"));
>          return -1;
> @@ -72,13 +76,24 @@ static int virIdentityOnceInit(void)
>  
>  VIR_ONCE_GLOBAL_INIT(virIdentity);
>  
> +static void virIdentity_init(virIdentity *ident G_GNUC_UNUSED)
> +{
> +}
> +
> +static void virIdentity_class_init(virIdentityClass *klass)
> +{
> +    GObjectClass *obj = G_OBJECT_CLASS(klass);
> +
> +    obj->finalize = virIdentityDispose;

Here we correctly use finalize so lets match the function name to it as
well.

Otherwise looks good.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list