[libvirt] [RFC 0/1] convert virStorageSource to GObject
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
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@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
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.
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@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 :|
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@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
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@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 :|
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@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
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Peter Krempa