Re: [libvirt PATCH] API: discourage usage of non-ListAll APIs
On Mon, 2020-10-05 at 00:35 +0200, Ján Tomko wrote:
+++ b/src/libvirt-domain-snapshot.c
@@ -378,8 +378,10 @@ virDomainSnapshotNum(virDomainPtr domain, unsigned int flags)
* snapshots were listed if the return is less than @nameslen. Likewise,
* you should be prepared for virDomainSnapshotLookupByName() to fail when
* converting a name from this call into a snapshot object, if another
- * connection deletes the snapshot in the meantime. For more control over
- * the results, see virDomainListAllSnapshots().
+ * connection deletes the snapshot in the meantime.
+ *
+ * The use of this function in new code is discouraged. Instead, use
+ * virDomainListAllSnapshots().
I would tweak this slightly to
The use of this function is discouraged, use Replacement() instead.
My argument for removing the "in new code" bit is that use of the old
APIs is a code smell, and even if everything appears to work fine
there are probably subtle bugs hiding right underneath the surface,
and so migrating to the newer APIs should be considered even for
existing libvirt-using projects.
Also I believe you missed virDomainSnapshotListChildrenNames().
+++ b/src/libvirt-nodedev.c
@@ -151,7 +151,8 @@ virConnectListAllNodeDevices(virConnectPtr conn,
*
* Collect the list of node devices, and store their names in @names
*
- * For more control over the results, see virConnectListAllNodeDevices().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllNodeDevices().
Unrelated to your patch, but we should probably introduce a
virNodeDeviceListAllCaps() API here.
+++ b/src/libvirt-nwfilter.c
@@ -117,6 +117,9 @@ virConnectListAllNWFilters(virConnectPtr conn,
*
* Collect the list of network filters, and store their names in @names
*
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListALLNWFilters().
s/ALL/All/
Should we consider adding the same notice to
* virDomainSnapshotNum()
* virDomainSnapshotNumChildren()
* virConnectNumOfDefinedDomains()
* virConnectNumOfDomains()
* virConnectNumOfInterfaces()
* virConnectNumOfDefinedInterfaces()
* virConnectNumOfNetworks()
* virConnectNumOfDefinedNetworks()
* virNodeNumOfDevices()
* virConnectNumOfNWFilters()
* virConnectNumOfSecrets()
* virConnectNumOfStoragePools()
* virConnectNumOfDefinedStoragePools()
* virStoragePoolNumOfVolumes()
or is there a reasonable situation in which you might want to call
those APIs and then pass the result to something other than the
various List() APIs?
Overally looks good! Thank you for taking the time to do this :)
--
Andrea Bolognani / Red Hat / Virtualization