7:42 a.m.
I was hoping to quickly re-send the qemu_driver cleanups I've
sent some time ago, now using Glib. I started by attempting to
change the first VIR_AUTOUNREF() call in qemu_driver.c to
g_autoptr(), which happens to be a virStorageSourcePtr type,
then I realized that it wasn't that simple.
Following up the instructions found on commit 16121a88a7, I started
the conversion. Then 'make check' started to fail because some
calls to virObjectRef/virObjectUnref were still remaining
in the code, messing up stuff related with mirrorChain in
qemu_blockjob.c. Turns out it was easier to burn through all the
instances and change them to use GLib.
This is being sent as RFC because x-I am aware that docs/hacking.html
mentions that we shouldn't mix up certain GLib macros with Libvirt
ones, thus I am uncertain of whether I have messed up or not.
'make check' works, did a few sanity checks with libvirtd as
well.
Hopefully this is somewhere near the mark. I intend to do such
convertions from time to time, based on the cleanups I wanted to
make in the qemu_driver file prior to the GLib introduction.
Daniel Henrique Barboza (1):
util: convert virStorageSource class to use GObject
src/conf/domain_conf.c | 13 ++---
src/conf/snapshot_conf.c | 3 +-
src/qemu/qemu_blockjob.c | 43 ++++++--------
src/qemu/qemu_domain.c | 6 +-
src/qemu/qemu_driver.c | 14 ++---
src/qemu/qemu_hotplug.c | 3 +-
src/qemu/qemu_migration.c | 2 +-
src/storage/storage_backend_gluster.c | 2 +-
src/storage/storage_util.c | 4 +-
src/util/virstoragefile.c | 84 +++++++++++++--------------
src/util/virstoragefile.h | 9 ++-
tests/qemublocktest.c | 6 +-
tests/virstoragetest.c | 12 ++--
13 files changed, 98 insertions(+), 103 deletions(-)
--
2.21.0
7:42 a.m.
New subject: [libvirt] [RFC 1/1] util: convert virStorageSource class to use GObject
Following up the directions provided in commit 16121a88a7, this
patch converts virStorageSource to use GObject.
All calls to virObjectRef were converted to g_object_ref. Calls to
virObjectUnref were changed to use either g_object_unref or
g_clear_object, depending on the context.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 13 ++---
src/conf/snapshot_conf.c | 3 +-
src/qemu/qemu_blockjob.c | 43 ++++++--------
src/qemu/qemu_domain.c | 6 +-
src/qemu/qemu_driver.c | 14 ++---
src/qemu/qemu_hotplug.c | 3 +-
src/qemu/qemu_migration.c | 2 +-
src/storage/storage_backend_gluster.c | 2 +-
src/storage/storage_util.c | 4 +-
src/util/virstoragefile.c | 84 +++++++++++++--------------
src/util/virstoragefile.h | 9 ++-
tests/qemublocktest.c | 6 +-
tests/virstoragetest.c | 12 ++--
13 files changed, 98 insertions(+), 103 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c1705a07b6..71888ebaf0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2097,10 +2097,10 @@ virDomainDiskDefFree(virDomainDiskDefPtr def)
if (!def)
return;
- virObjectUnref(def->src);
+ g_clear_object(&def->src);
VIR_FREE(def->serial);
VIR_FREE(def->dst);
- virObjectUnref(def->mirror);
+ g_clear_object(&def->mirror);
VIR_FREE(def->wwn);
VIR_FREE(def->driverName);
VIR_FREE(def->vendor);
@@ -2312,7 +2312,7 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
if (!def)
return;
- virObjectUnref(def->src);
+ g_object_unref(def->src);
VIR_FREE(def->dst);
virDomainDeviceInfoClear(&def->info);
VIR_FREE(def->virtio);
@@ -2896,8 +2896,7 @@
virDomainHostdevSubsysSCSIiSCSIClear(virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc
if (!iscsisrc)
return;
- virObjectUnref(iscsisrc->src);
- iscsisrc->src = NULL;
+ g_clear_object(&iscsisrc->src);
}
@@ -9276,7 +9275,7 @@ virDomainStorageSourceParseBase(const char *type,
const char *format,
const char *index)
{
- VIR_AUTOUNREF(virStorageSourcePtr) src = NULL;
+ g_autoptr(virStorageSource) src = NULL;
virStorageSourcePtr ret = NULL;
if (!(src = virStorageSourceNew()))
@@ -9402,7 +9401,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
{
VIR_XPATH_NODE_AUTORESTORE(ctxt);
xmlNodePtr source;
- VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL;
+ g_autoptr(virStorageSource) backingStore = NULL;
VIR_AUTOFREE(char *) type = NULL;
VIR_AUTOFREE(char *) format = NULL;
VIR_AUTOFREE(char *) idx = NULL;
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index a77f521302..cb85d147b8 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -94,8 +94,7 @@ static void
virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk)
{
VIR_FREE(disk->name);
- virObjectUnref(disk->src);
- disk->src = NULL;
+ g_clear_object(&disk->src);
}
void
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index c118f2c298..5bac0ec976 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -76,11 +76,11 @@ qemuBlockJobDataDispose(void *obj)
{
qemuBlockJobDataPtr job = obj;
- virObjectUnref(job->chain);
- virObjectUnref(job->mirrorChain);
+ g_clear_object(&job->chain);
+ g_clear_object(&job->mirrorChain);
if (job->type == QEMU_BLOCKJOB_TYPE_CREATE)
- virObjectUnref(job->data.create.src);
+ g_clear_object(&job->data.create.src);
VIR_FREE(job->name);
VIR_FREE(job->errmsg);
@@ -156,7 +156,7 @@ qemuBlockJobRegister(qemuBlockJobDataPtr job,
if (disk) {
job->disk = disk;
- job->chain = virObjectRef(disk->src);
+ job->chain = g_object_ref(disk->src);
QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = virObjectRef(job);
}
@@ -304,9 +304,9 @@ qemuBlockJobNewCreate(virDomainObjPtr vm,
return NULL;
if (virStorageSourceIsBacking(chain))
- job->chain = virObjectRef(chain);
+ job->chain = g_object_ref(chain);
- job->data.create.src = virObjectRef(src);
+ job->data.create.src = g_object_ref(src);
if (qemuBlockJobRegister(job, vm, NULL, true) < 0)
return NULL;
@@ -337,7 +337,7 @@ qemuBlockJobDiskNewCopy(virDomainObjPtr vm,
if (!(job = qemuBlockJobDataNew(QEMU_BLOCKJOB_TYPE_COPY, jobname)))
return NULL;
- job->mirrorChain = virObjectRef(mirror);
+ job->mirrorChain = g_object_ref(mirror);
if (shallow && !reuse)
job->data.copy.shallownew = true;
@@ -595,7 +595,7 @@ qemuBlockJobRewriteConfigDiskSource(virDomainObjPtr vm,
virStorageSourcePtr newsrc)
{
virDomainDiskDefPtr persistDisk = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) copy = NULL;
+ g_autoptr(virStorageSource) copy = NULL;
virStorageSourcePtr n;
if (!vm->newDef)
@@ -626,7 +626,7 @@ qemuBlockJobRewriteConfigDiskSource(virDomainObjPtr vm,
}
}
- virObjectUnref(persistDisk->src);
+ g_object_unref(persistDisk->src);
VIR_STEAL_PTR(persistDisk->src, copy);
}
@@ -661,7 +661,7 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
disk->dst);
}
- virObjectUnref(disk->src);
+ g_object_unref(disk->src);
disk->src = disk->mirror;
} else {
virStorageSourcePtr n;
@@ -682,7 +682,7 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
}
}
- virObjectUnref(disk->mirror);
+ g_object_unref(disk->mirror);
}
for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
@@ -777,8 +777,7 @@ qemuBlockJobEventProcessLegacy(virQEMUDriverPtr driver,
}
}
- virObjectUnref(disk->mirror);
- disk->mirror = NULL;
+ g_clear_object(&disk->mirror);
}
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
@@ -957,14 +956,14 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
if (baseparent)
baseparent->backingStore = NULL;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob, tmp);
- virObjectUnref(tmp);
+ g_object_unref(tmp);
if (cfgdisk) {
tmp = cfgdisk->src->backingStore;
cfgdisk->src->backingStore = cfgbase;
if (cfgbaseparent)
cfgbaseparent->backingStore = NULL;
- virObjectUnref(tmp);
+ g_object_unref(tmp);
}
}
@@ -1126,8 +1125,7 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr
driver,
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
/* the mirror element does not serve functional purpose for the commit job */
- virObjectUnref(job->disk->mirror);
- job->disk->mirror = NULL;
+ g_clear_object(&job->disk->mirror);
}
@@ -1153,7 +1151,7 @@ qemuBlockJobProcessEventConcludedCopyPivot(virQEMUDriverPtr driver,
qemuBlockJobRewriteConfigDiskSource(vm, job->disk, job->disk->mirror);
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->disk->src);
- virObjectUnref(job->disk->src);
+ g_object_unref(job->disk->src);
VIR_STEAL_PTR(job->disk->src, job->disk->mirror);
}
@@ -1170,8 +1168,7 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver,
return;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->disk->mirror);
- virObjectUnref(job->disk->mirror);
- job->disk->mirror = NULL;
+ g_clear_object(&job->disk->mirror);
}
@@ -1201,8 +1198,7 @@ qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver,
}
}
- virObjectUnref(disk->mirror);
- disk->mirror = NULL;
+ g_clear_object(&disk->mirror);
}
@@ -1218,8 +1214,7 @@ qemuBlockJobProcessEventConcludedCreate(virQEMUDriverPtr driver,
* it will handle further hotplug of the created volume and also that
* the 'chain' which was registered is under their control */
if (job->synchronous) {
- virObjectUnref(job->chain);
- job->chain = NULL;
+ g_clear_object(&job->chain);
return;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 35067c851f..8f97db3db6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3015,7 +3015,7 @@ qemuDomainObjPrivateXMLParseBlockjobChain(xmlNodePtr node,
VIR_AUTOFREE(char *) format = NULL;
VIR_AUTOFREE(char *) type = NULL;
VIR_AUTOFREE(char *) index = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) src = NULL;
+ g_autoptr(virStorageSource) src = NULL;
xmlNodePtr sourceNode;
unsigned int xmlflags = VIR_DOMAIN_DEF_PARSE_STATUS;
@@ -3220,7 +3220,7 @@ qemuDomainObjPrivateXMLParseBlockjobData(virDomainObjPtr vm,
if (mirror) {
if (disk)
- job->mirrorChain = virObjectRef(disk->mirror);
+ job->mirrorChain = g_object_ref(disk->mirror);
else
invalidData = true;
}
@@ -3313,7 +3313,7 @@ qemuDomainObjPrivateXMLParseJobNBDSource(xmlNodePtr node,
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
VIR_AUTOFREE(char *) format = NULL;
VIR_AUTOFREE(char *) type = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) migrSource = NULL;
+ g_autoptr(virStorageSource) migrSource = NULL;
xmlNodePtr sourceNode;
ctxt->node = node;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc0ede2fb0..237b205fef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -255,7 +255,7 @@ qemuSecurityChownCallback(const virStorageSource *src,
int save_errno = 0;
int ret = -1;
int rv;
- VIR_AUTOUNREF(virStorageSourcePtr) cpy = NULL;
+ g_autoptr(virStorageSource) cpy = NULL;
rv = virStorageFileSupportsSecurityDriver(src);
if (rv <= 0)
@@ -15424,9 +15424,9 @@ qemuDomainSnapshotDiskCleanup(qemuDomainSnapshotDiskDataPtr data,
if (data[i].prepared)
qemuDomainStorageSourceAccessRevoke(driver, vm, data[i].src);
- virObjectUnref(data[i].src);
+ g_object_unref(data[i].src);
}
- virObjectUnref(data[i].persistsrc);
+ g_object_unref(data[i].persistsrc);
VIR_FREE(data[i].relPath);
qemuBlockStorageSourceChainDataFree(data[i].crdata);
}
@@ -15450,7 +15450,7 @@ qemuDomainSnapshotDiskPrepareOne(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = vm->privateData;
char *backingStoreStr;
virDomainDiskDefPtr persistdisk;
- VIR_AUTOUNREF(virStorageSourcePtr) terminator = NULL;
+ g_autoptr(virStorageSource) terminator = NULL;
bool supportsCreate;
bool supportsBacking;
int rc;
@@ -18212,7 +18212,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW);
bool existing = mirror_reuse;
qemuBlockJobDataPtr job = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc;
+ g_autoptr(virStorageSource) mirror = mirrorsrc;
bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
bool mirror_initialized = false;
VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL;
@@ -18485,7 +18485,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const
char *base,
virDomainObjPtr vm;
int ret = -1;
unsigned long long speed = bandwidth;
- VIR_AUTOUNREF(virStorageSourcePtr) dest = NULL;
+ g_autoptr(virStorageSource) dest = NULL;
virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW |
VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT |
@@ -18690,7 +18690,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
VIR_AUTOFREE(char *) backingPath = NULL;
unsigned long long speed = bandwidth;
qemuBlockJobDataPtr job = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) mirror = NULL;
+ g_autoptr(virStorageSource) mirror = NULL;
const char *nodetop = NULL;
const char *nodebase = NULL;
bool persistjob = false;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index e9285f0964..674afb7247 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -636,8 +636,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, oldsrc));
/* media was changed, so we can remove the old media definition now */
- virObjectUnref(oldsrc);
- oldsrc = NULL;
+ g_clear_object(&oldsrc);
disk->src = newsrc;
ret = 0;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a98ec2d55a..a21e1f8e01 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -797,7 +797,7 @@ qemuMigrationSrcNBDStorageCopyBlockdev(virQEMUDriverPtr driver,
VIR_AUTOPTR(qemuBlockStorageSourceAttachData) data = NULL;
qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
int mon_ret = 0;
- VIR_AUTOUNREF(virStorageSourcePtr) copysrc = NULL;
+ g_autoptr(virStorageSource) copysrc = NULL;
VIR_DEBUG("starting blockdev mirror for disk=%s to host=%s", diskAlias,
host);
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index 5955d834d9..8cb822f0cf 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -239,7 +239,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
ssize_t len;
int backingFormat;
VIR_AUTOPTR(virStorageVolDef) vol = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) meta = NULL;
+ g_autoptr(virStorageSourcePtr) meta = NULL;
VIR_AUTOFREE(char *) header = NULL;
*volptr = NULL;
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index be084119f4..c5aab148f2 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -3359,7 +3359,7 @@ storageBackendProbeTarget(virStorageSourcePtr target,
int backingStoreFormat;
int rc;
struct stat sb;
- VIR_AUTOUNREF(virStorageSourcePtr) meta = NULL;
+ g_autoptr(virStorageSource) meta = NULL;
VIR_AUTOCLOSE fd = -1;
if (encryption)
@@ -3529,7 +3529,7 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
int ret = -1;
VIR_AUTOPTR(virStorageVolDef) vol = NULL;
VIR_AUTOCLOSE fd = -1;
- VIR_AUTOUNREF(virStorageSourcePtr) target = NULL;
+ g_autoptr(virStorageSource) target = NULL;
if (virDirOpen(&dir, def->target.path) < 0)
goto cleanup;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 497ade927e..568e310290 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -47,7 +47,38 @@
VIR_LOG_INIT("util.storagefile");
-static virClassPtr virStorageSourceClass;
+G_DEFINE_TYPE(virStorageSource, vir_storagesource, G_TYPE_OBJECT)
+
+static void vir_storagesource_init(virStorageSource *ident G_GNUC_UNUSED)
+{
+}
+
+
+static void
+virStorageSourceFinalize(GObject *obj)
+{
+ virStorageSourcePtr src = VIR_STORAGESOURCE(obj);
+
+ virStorageSourceClear(src);
+
+ G_OBJECT_CLASS(vir_storagesource_parent_class)->finalize(obj);
+}
+
+
+static void vir_storagesource_class_init(virStorageSourceClass *klass)
+{
+ GObjectClass *obj = G_OBJECT_CLASS(klass);
+
+ obj->finalize = virStorageSourceFinalize;
+}
+
+
+virStorageSourcePtr
+virStorageSourceNew(void)
+{
+ return VIR_STORAGESOURCE(g_object_new(VIR_TYPE_STORAGESOURCE, NULL));
+}
+
VIR_ENUM_IMPL(virStorage,
VIR_STORAGE_TYPE_LAST,
@@ -1129,7 +1160,7 @@ static virStorageSourcePtr
virStorageFileMetadataNew(const char *path,
int format)
{
- VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+ g_autoptr(virStorageSource) def = NULL;
virStorageSourcePtr ret = NULL;
if (!(def = virStorageSourceNew()))
@@ -1219,7 +1250,7 @@ virStorageFileGetMetadataFromFD(const char *path,
struct stat sb;
int dummy;
VIR_AUTOFREE(char *) buf = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) meta = NULL;
+ g_autoptr(virStorageSource) meta = NULL;
if (!backingFormat)
backingFormat = &dummy;
@@ -2251,7 +2282,7 @@ virStorageSourceCopy(const virStorageSource *src,
bool backingChain)
{
virStorageSourcePtr ret = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+ g_autoptr(virStorageSource) def = NULL;
if (!(def = virStorageSourceNew()))
return NULL;
@@ -2591,37 +2622,6 @@ virStorageSourceClear(virStorageSourcePtr def)
}
-static void
-virStorageSourceDispose(void *obj)
-{
- virStorageSourcePtr src = obj;
-
- virStorageSourceClear(src);
-}
-
-
-static int
-virStorageSourceOnceInit(void)
-{
- if (!VIR_CLASS_NEW(virStorageSource, virClassForObject()))
- return -1;
-
- return 0;
-}
-
-
-VIR_ONCE_GLOBAL_INIT(virStorageSource);
-
-
-virStorageSourcePtr
-virStorageSourceNew(void)
-{
- if (virStorageSourceInitialize() < 0)
- return NULL;
-
- return virObjectNew(virStorageSourceClass);
-}
-
static virStorageSourcePtr
virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
@@ -2629,7 +2629,7 @@ virStorageSourceNewFromBackingRelative(virStorageSourcePtr parent,
{
virStorageSourcePtr ret = NULL;
VIR_AUTOFREE(char *) dirname = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+ g_autoptr(virStorageSource) def = NULL;
if (!(def = virStorageSourceNew()))
return NULL;
@@ -3681,7 +3681,7 @@ virStorageSourceNewFromBackingAbsolute(const char *path,
{
const char *json;
int rc = 0;
- VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+ g_autoptr(virStorageSource) def = NULL;
*src = NULL;
@@ -3748,7 +3748,7 @@ virStorageSourceNewFromChild(virStorageSourcePtr parent,
virStorageSourcePtr *child)
{
struct stat st;
- VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+ g_autoptr(virStorageSource) def = NULL;
int rc = 0;
*child = NULL;
@@ -3948,7 +3948,7 @@ virStorageSourceUpdateCapacity(virStorageSourcePtr src,
bool probe)
{
int format = src->format;
- VIR_AUTOUNREF(virStorageSourcePtr) meta = NULL;
+ g_autoptr(virStorageSource) meta = NULL;
/* Raw files: capacity is physical size. For all other files: if
* the metadata has a capacity, use that, otherwise fall back to
@@ -4943,7 +4943,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
int backingFormat;
int rv;
VIR_AUTOFREE(char *) buf = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) backingStore = NULL;
+ g_autoptr(virStorageSource) backingStore = NULL;
VIR_DEBUG("path=%s format=%d uid=%u gid=%u",
src->path, src->format,
@@ -5028,7 +5028,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
VIR_STEAL_PTR(src->backingStore, backingStore);
if (src->externalDataStoreRaw) {
- VIR_AUTOUNREF(virStorageSourcePtr) externalDataStore = NULL;
+ g_autoptr(virStorageSource) externalDataStore = NULL;
if ((rv = virStorageSourceNewFromExternalData(src,
&externalDataStore)) < 0)
@@ -5121,7 +5121,7 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src,
ssize_t headerLen;
int rv;
VIR_AUTOFREE(char *) buf = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) tmp = NULL;
+ g_autoptr(virStorageSource) tmp = NULL;
*backing = NULL;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index cfee047e6f..b4eca6448d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -21,8 +21,10 @@
#pragma once
+#include <glib-object.h>
#include <sys/stat.h>
+#include "internal.h"
#include "virbitmap.h"
#include "virobject.h"
#include "virseclabel.h"
@@ -234,7 +236,9 @@ struct _virStorageSourceInitiatorDef {
typedef struct _virStorageDriverData virStorageDriverData;
typedef virStorageDriverData *virStorageDriverDataPtr;
-typedef struct _virStorageSource virStorageSource;
+#define VIR_TYPE_STORAGESOURCE vir_storagesource_get_type()
+G_DECLARE_FINAL_TYPE(virStorageSource, vir_storagesource, VIR, STORAGESOURCE, GObject);
+
typedef virStorageSource *virStorageSourcePtr;
/* Stores information related to a host resource. In the case of backing
@@ -243,7 +247,7 @@ typedef virStorageSource *virStorageSourcePtr;
* IMPORTANT: When adding fields to this struct it's also necessary to add
* appropriate code to the virStorageSourceCopy deep copy function */
struct _virStorageSource {
- virObject parent;
+ GObject parent;
unsigned int id; /* backing chain identifier, 0 is unset */
int type; /* virStorageType */
@@ -344,7 +348,6 @@ struct _virStorageSource {
bool hostcdrom; /* backing device is a cdrom */
};
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref);
#ifndef DEV_BSIZE
diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index bb0579056e..cd4d0edf45 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -54,8 +54,8 @@ testBackingXMLjsonXML(const void *args)
VIR_AUTOFREE(char *) propsstr = NULL;
VIR_AUTOFREE(char *) protocolwrapper = NULL;
VIR_AUTOFREE(char *) actualxml = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) xmlsrc = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) jsonsrc = NULL;
+ g_autoptr(virStorageSource) xmlsrc = NULL;
+ g_autoptr(virStorageSource) jsonsrc = NULL;
if (!(xmlsrc = virStorageSourceNew()))
return -1;
@@ -400,7 +400,7 @@ testQemuImageCreate(const void *opaque)
struct testQemuImageCreateData *data = (void *) opaque;
VIR_AUTOPTR(virJSONValue) protocolprops = NULL;
VIR_AUTOPTR(virJSONValue) formatprops = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) src = NULL;
+ g_autoptr(virStorageSource) src = NULL;
VIR_AUTOCLEAN(virBuffer) debug = VIR_BUFFER_INITIALIZER;
VIR_AUTOCLEAN(virBuffer) actualbuf = VIR_BUFFER_INITIALIZER;
VIR_AUTOFREE(char *) jsonprotocol = NULL;
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 8ebad89da7..f178e66781 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -86,7 +86,7 @@ testStorageFileGetMetadata(const char *path,
{
struct stat st;
virStorageSourcePtr ret = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) def = NULL;
+ g_autoptr(virStorageSource) def = NULL;
if (!(def = virStorageSourceNew()))
return NULL;
@@ -278,7 +278,7 @@ testStorageChain(const void *args)
const struct testChainData *data = args;
virStorageSourcePtr elt;
size_t i = 0;
- VIR_AUTOUNREF(virStorageSourcePtr) meta = NULL;
+ g_autoptr(virStorageSource) meta = NULL;
VIR_AUTOFREE(char *) broken = NULL;
meta = testStorageFileGetMetadata(data->start, data->format, -1, -1);
@@ -612,7 +612,7 @@ testBackingParse(const void *args)
const struct testBackingParseData *data = args;
VIR_AUTOCLEAN(virBuffer) buf = VIR_BUFFER_INITIALIZER;
VIR_AUTOFREE(char *) xml = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) src = NULL;
+ g_autoptr(virStorageSource) src = NULL;
int rc;
int erc = data->rv;
@@ -664,7 +664,7 @@ mymain(void)
virStorageSourcePtr chain2; /* short for chain->backingStore */
virStorageSourcePtr chain3; /* short for chain2->backingStore */
VIR_AUTOPTR(virCommand) cmd = NULL;
- VIR_AUTOUNREF(virStorageSourcePtr) chain = NULL;
+ g_autoptr(virStorageSource) chain = NULL;
if (storageRegisterAll() < 0)
return EXIT_FAILURE;
@@ -1056,7 +1056,7 @@ mymain(void)
ret = -1;
/* Test behavior of chain lookups, relative backing from absolute start */
- virObjectUnref(chain);
+ g_object_unref(chain);
chain = testStorageFileGetMetadata(abswrap, VIR_STORAGE_FILE_QCOW2, -1, -1);
if (!chain) {
ret = -1;
@@ -1102,7 +1102,7 @@ mymain(void)
ret = -1;
/* Test behavior of chain lookups, relative backing */
- virObjectUnref(chain);
+ g_object_unref(chain);
chain = testStorageFileGetMetadata("sub/link2", VIR_STORAGE_FILE_QCOW2,
-1, -1);
if (!chain) {
--
2.21.0
7:53 a.m.
On Tue, Oct 15, 2019 at 09:42:45 -0300, Daniel Henrique Barboza wrote:
I was hoping to quickly re-send the qemu_driver cleanups I've
sent some time ago, now using Glib. I started by attempting to
change the first VIR_AUTOUNREF() call in qemu_driver.c to
g_autoptr(), which happens to be a virStorageSourcePtr type,
then I realized that it wasn't that simple.
Following up the instructions found on commit 16121a88a7, I started
the conversion. Then 'make check' started to fail because some
calls to virObjectRef/virObjectUnref were still remaining
in the code, messing up stuff related with mirrorChain in
qemu_blockjob.c. Turns out it was easier to burn through all the
instances and change them to use GLib.
This is being sent as RFC because x-I am aware that docs/hacking.html
mentions that we shouldn't mix up certain GLib macros with Libvirt
ones, thus I am uncertain of whether I have messed up or not.
'make check' works, did a few sanity checks with libvirtd as
well.
Hopefully this is somewhere near the mark. I intend to do such
convertions from time to time, based on the cleanups I wanted to
make in the qemu_driver file prior to the GLib introduction.
I'd prefer if this kind of experiments is done on something simpler than
virStorageSource.
7:55 a.m.
On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote:
I was hoping to quickly re-send the qemu_driver cleanups I've
sent some time ago, now using Glib. I started by attempting to
change the first VIR_AUTOUNREF() call in qemu_driver.c to
g_autoptr(), which happens to be a virStorageSourcePtr type,
then I realized that it wasn't that simple.
It should be that simple with this commit:
commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Fri Oct 4 17:14:10 2019 +0100
src: add support for g_autoptr with virObject instances
we should be able to use g_autoptr for any virObject, without
having to lock-step convert to GObject.
What actual problem did you find ?
Following up the instructions found on commit 16121a88a7, I started
the conversion. Then 'make check' started to fail because some
calls to virObjectRef/virObjectUnref were still remaining
in the code, messing up stuff related with mirrorChain in
qemu_blockjob.c. Turns out it was easier to burn through all the
instances and change them to use GLib.
Yes, if you convert from virObject to GObject, you *must*
convert all virObjectRef/Unref calls at that time.
This is being sent as RFC because x-I am aware that
docs/hacking.html
mentions that we shouldn't mix up certain GLib macros with Libvirt
ones, thus I am uncertain of whether I have messed up or not.
'make check' works, did a few sanity checks with libvirtd as
well.
Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit
667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr
with virObject, without first converting to GObject.
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 :|
8:51 a.m.
On 10/15/19 9:55 AM, Daniel P. Berrangé wrote:
On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza
wrote:
> I was hoping to quickly re-send the qemu_driver cleanups I've
> sent some time ago, now using Glib. I started by attempting to
> change the first VIR_AUTOUNREF() call in qemu_driver.c to
> g_autoptr(), which happens to be a virStorageSourcePtr type,
> then I realized that it wasn't that simple.
It should be that simple with this commit:
commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a
Author: Daniel P. Berrangé <berrange(a)redhat.com>
Date: Fri Oct 4 17:14:10 2019 +0100
src: add support for g_autoptr with virObject instances
we should be able to use g_autoptr for any virObject, without
having to lock-step convert to GObject.
What actual problem did you find ?
I failed to notice this commit. Just tried it again and it worked.
What happened yesterday was that I attempted to do a simple
VIR_AUTOUNREF -> g_autopt replace, faced compile errors and then, since
I didn't notice this commit about, I assumed "I guess I need to convert
this guy to GObject".
In fact, the compile error happened because g_autoptr() does not operate
with a 'Ptr' type - something that I learned only during the conversion
process.
Well, hopefully this patch can serve as a baseline for a future
conversion for
this object type. Guess I can go back safely to re-send the cleanup
patches tha
are already pending in the ML hehehe
> Following up the instructions found on commit 16121a88a7, I started
> the conversion. Then 'make check' started to fail because some
> calls to virObjectRef/virObjectUnref were still remaining
> in the code, messing up stuff related with mirrorChain in
> qemu_blockjob.c. Turns out it was easier to burn through all the
> instances and change them to use GLib.
Yes, if you convert from virObject to GObject, you *must*
convert all virObjectRef/Unref calls at that time.
> This is being sent as RFC because x-I am aware that docs/hacking.html
> mentions that we shouldn't mix up certain GLib macros with Libvirt
> ones, thus I am uncertain of whether I have messed up or not.
> 'make check' works, did a few sanity checks with libvirtd as
> well.
Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit
667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr
with virObject, without first converting to GObject.
What if there are other object types in the same function using the VIR
macros?
For example, inside qemu_driver.c: qemuDomainBlockCopyCommon:
VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
virQEMUDriverGetConfig(driver);
const char *format = NULL;
bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW);
bool existing = mirror_reuse;
qemuBlockJobDataPtr job = NULL;
VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc;
bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
bool mirror_initialized = false;
VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL;
VIR_AUTOPTR(qemuBlockStorageSourceChainData) crdata = NULL;
Let's say that I change the virStorageSourcePtr up there to
g_autoptr(virStorageSource) mirror = mirrorsrc;
As long as there are no VIR macros acting in the 'mirror' variable, is
it to use g_autoptr
there even when everyone else is using VIR_AUTO* macros?
Thanks,
DHB
Regards,
Daniel
12:15 p.m.
On Tue, Oct 15, 2019 at 10:51:39AM -0300, Daniel Henrique Barboza wrote:
On 10/15/19 9:55 AM, Daniel P. Berrangé wrote:
> On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote:
> > I was hoping to quickly re-send the qemu_driver cleanups I've
> > sent some time ago, now using Glib. I started by attempting to
> > change the first VIR_AUTOUNREF() call in qemu_driver.c to
> > g_autoptr(), which happens to be a virStorageSourcePtr type,
> > then I realized that it wasn't that simple.
> It should be that simple with this commit:
>
> commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a
> Author: Daniel P. Berrangé <berrange(a)redhat.com>
> Date: Fri Oct 4 17:14:10 2019 +0100
>
> src: add support for g_autoptr with virObject instances
>
> we should be able to use g_autoptr for any virObject, without
> having to lock-step convert to GObject.
>
> What actual problem did you find ?
I failed to notice this commit. Just tried it again and it worked.
What happened yesterday was that I attempted to do a simple
VIR_AUTOUNREF -> g_autopt replace, faced compile errors and then, since
I didn't notice this commit about, I assumed "I guess I need to convert
this guy to GObject".
In fact, the compile error happened because g_autoptr() does not operate
with a 'Ptr' type - something that I learned only during the conversion
process.
Yeah, you need to drop the 'Ptr' suffix in the type name when
converting to g_autoptr, as it adds the pointer itself.
> > This is being sent as RFC because x-I am aware that
docs/hacking.html
> > mentions that we shouldn't mix up certain GLib macros with Libvirt
> > ones, thus I am uncertain of whether I have messed up or not.
> > 'make check' works, did a few sanity checks with libvirtd as
> > well.
> Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit
> 667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr
> with virObject, without first converting to GObject.
What if there are other object types in the same function using the VIR
macros?
For example, inside qemu_driver.c: qemuDomainBlockCopyCommon:
VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
virQEMUDriverGetConfig(driver);
const char *format = NULL;
bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW);
bool existing = mirror_reuse;
qemuBlockJobDataPtr job = NULL;
VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc;
bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
bool mirror_initialized = false;
VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL;
VIR_AUTOPTR(qemuBlockStorageSourceChainData) crdata = NULL;
Let's say that I change the virStorageSourcePtr up there to
g_autoptr(virStorageSource) mirror = mirrorsrc;
As long as there are no VIR macros acting in the 'mirror' variable, is it to
use g_autoptr
there even when everyone else is using VIR_AUTO* macros?
You should change all variables in the method at the same time.
Both the VIR_AUTOUNEF calls here can use g_autoptr, as can the
two VIR_AUTOPTR calls.
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 :|
12:43 p.m.
On 10/15/19 2:15 PM, Daniel P. Berrangé wrote:
On Tue, Oct 15, 2019 at 10:51:39AM -0300, Daniel Henrique Barboza
wrote:
>
> On 10/15/19 9:55 AM, Daniel P. Berrangé wrote:
>> On Tue, Oct 15, 2019 at 09:42:45AM -0300, Daniel Henrique Barboza wrote:
>>> I was hoping to quickly re-send the qemu_driver cleanups I've
>>> sent some time ago, now using Glib. I started by attempting to
>>> change the first VIR_AUTOUNREF() call in qemu_driver.c to
>>> g_autoptr(), which happens to be a virStorageSourcePtr type,
>>> then I realized that it wasn't that simple.
>> It should be that simple with this commit:
>>
>> commit 667ff797e8eb8d82f30ab430216a8d2eef6b915a
>> Author: Daniel P. Berrangé <berrange(a)redhat.com>
>> Date: Fri Oct 4 17:14:10 2019 +0100
>>
>> src: add support for g_autoptr with virObject instances
>>
>> we should be able to use g_autoptr for any virObject, without
>> having to lock-step convert to GObject.
>>
>> What actual problem did you find ?
> I failed to notice this commit. Just tried it again and it worked.
>
> What happened yesterday was that I attempted to do a simple
> VIR_AUTOUNREF -> g_autopt replace, faced compile errors and then, since
> I didn't notice this commit about, I assumed "I guess I need to convert
> this guy to GObject".
>
> In fact, the compile error happened because g_autoptr() does not operate
> with a 'Ptr' type - something that I learned only during the conversion
> process.
Yeah, you need to drop the 'Ptr' suffix in the type name when
converting to g_autoptr, as it adds the pointer itself.
>>> This is being sent as RFC because x-I am aware that docs/hacking.html
>>> mentions that we shouldn't mix up certain GLib macros with Libvirt
>>> ones, thus I am uncertain of whether I have messed up or not.
>>> 'make check' works, did a few sanity checks with libvirtd as
>>> well.
>> Yes, the need to not mix g_auto* with VIR_AUTO*, is why I did commit
>> 667ff797e8eb8d82f30ab430216a8d2eef6b915a to let you use g_autoptr
>> with virObject, without first converting to GObject.
> What if there are other object types in the same function using the VIR
> macros?
> For example, inside qemu_driver.c: qemuDomainBlockCopyCommon:
>
>
> VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg =
> virQEMUDriverGetConfig(driver);
> const char *format = NULL;
> bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
> bool mirror_shallow = !!(flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW);
> bool existing = mirror_reuse;
> qemuBlockJobDataPtr job = NULL;
> VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc;
> bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
> bool mirror_initialized = false;
> VIR_AUTOPTR(qemuBlockStorageSourceChainData) data = NULL;
> VIR_AUTOPTR(qemuBlockStorageSourceChainData) crdata = NULL;
>
>
> Let's say that I change the virStorageSourcePtr up there to
>
> g_autoptr(virStorageSource) mirror = mirrorsrc;
>
>
> As long as there are no VIR macros acting in the 'mirror' variable, is it to
> use g_autoptr
> there even when everyone else is using VIR_AUTO* macros?
You should change all variables in the method at the same time.
Both the VIR_AUTOUNEF calls here can use g_autoptr, as can the
two VIR_AUTOPTR calls.
Thanks for clarifying. I was going to re-send the patches adding GLib
macros instead of VIR_AUTO* ones, which would end up breaking this
rule because these patches are changing stuff in smaller steps.
What I'll end up is to basically re-send them as they are now, but with an
extra patch to change everything to GLib at once. That way we'll stay
compliant every step of the way.
DHB
Regards,
Daniel
1902
days inactive
1902
days old
6 comments
3 participants
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Peter Krempa