[libvirt PATCH] API: discourage usage of non-ListAll APIs

Ján Tomko posted 1 patch 3 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/0cb6de323e4a2e578deb92435c540ef87f610350.1601850918.git.jtomko@redhat.com
There is a newer version of this series
src/libvirt-domain-snapshot.c | 6 ++++--
src/libvirt-domain.c          | 8 ++++----
src/libvirt-interface.c       | 6 ++++--
src/libvirt-network.c         | 6 ++++--
src/libvirt-nodedev.c         | 3 ++-
src/libvirt-nwfilter.c        | 3 +++
src/libvirt-secret.c          | 3 +++
src/libvirt-storage.c         | 9 ++++++---
8 files changed, 30 insertions(+), 14 deletions(-)
[libvirt PATCH] API: discourage usage of non-ListAll APIs
Posted by Ján Tomko 3 years, 6 months ago
They require the caller to provide the maximum number
of array elements upfront, leading to either incomplete
results or violations of the zero-one-infinity rule.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 src/libvirt-domain-snapshot.c | 6 ++++--
 src/libvirt-domain.c          | 8 ++++----
 src/libvirt-interface.c       | 6 ++++--
 src/libvirt-network.c         | 6 ++++--
 src/libvirt-nodedev.c         | 3 ++-
 src/libvirt-nwfilter.c        | 3 +++
 src/libvirt-secret.c          | 3 +++
 src/libvirt-storage.c         | 9 ++++++---
 8 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index f856e5b9b8..74d5e16281 100644
--- a/src/libvirt-domain-snapshot.c
+++ 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().
  *
  * Returns the number of domain snapshots found or -1 in case of error.
  * The caller is responsible to call free() for each member of the array.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 415482a526..03eca1196a 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -41,8 +41,8 @@ VIR_LOG_INIT("libvirt.domain");
  *
  * Collect the list of active domains, and store their IDs in array @ids
  *
- * For inactive domains, see virConnectListDefinedDomains().  For more
- * control over the results, see virConnectListAllDomains().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllDomains().
  *
  * Returns the number of domains found or -1 in case of error.  Note that
  * this command is inherently racy; a domain can be started between a
@@ -6366,8 +6366,8 @@ virConnectNumOfDefinedDomains(virConnectPtr conn)
  * list the defined but inactive domains, stores the pointers to the names
  * in @names
  *
- * For active domains, see virConnectListDomains().  For more control over
- * the results, see virConnectListAllDomains().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllDomains().
  *
  * Returns the number of names provided in the array or -1 in case of error.
  * Note that this command is inherently racy; a domain can be defined between
diff --git a/src/libvirt-interface.c b/src/libvirt-interface.c
index 5eb5980483..271f21b7b5 100644
--- a/src/libvirt-interface.c
+++ b/src/libvirt-interface.c
@@ -150,7 +150,8 @@ virConnectNumOfInterfaces(virConnectPtr conn)
  * Collect the list of active physical host interfaces,
  * and store their names in @names
  *
- * For more control over the results, see virConnectListAllInterfaces().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllInterfaces().
  *
  * Returns the number of interfaces found or -1 in case of error.  Note that
  * this command is inherently racy; a interface can be started between a call
@@ -227,7 +228,8 @@ virConnectNumOfDefinedInterfaces(virConnectPtr conn)
  * Collect the list of defined (inactive) physical host interfaces,
  * and store their names in @names.
  *
- * For more control over the results, see virConnectListAllInterfaces().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllInterfaces().
  *
  * Returns the number of names provided in the array or -1 in case of error.
  * Note that this command is inherently racy; a interface can be defined between
diff --git a/src/libvirt-network.c b/src/libvirt-network.c
index f691b672c7..e6ea66a234 100644
--- a/src/libvirt-network.c
+++ b/src/libvirt-network.c
@@ -159,7 +159,8 @@ virConnectNumOfNetworks(virConnectPtr conn)
  *
  * Collect the list of active networks, and store their names in @names
  *
- * For more control over the results, see virConnectListAllNetworks().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllNetworks().
  *
  * Returns the number of networks found or -1 in case of error.  Note that
  * this command is inherently racy; a network can be started between a call
@@ -235,7 +236,8 @@ virConnectNumOfDefinedNetworks(virConnectPtr conn)
  *
  * list the inactive networks, stores the pointers to the names in @names
  *
- * For more control over the results, see virConnectListAllNetworks().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllNetworks().
  *
  * Returns the number of names provided in the array or -1 in case of error.
  * Note that this command is inherently racy; a network can be defined between
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index 28765b9c30..6b1e03c531 100644
--- a/src/libvirt-nodedev.c
+++ 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().
  *
  * If the optional 'cap'  argument is non-NULL, then the count
  * will be restricted to devices with the specified capability
diff --git a/src/libvirt-nwfilter.c b/src/libvirt-nwfilter.c
index e299385895..11397f2f4c 100644
--- a/src/libvirt-nwfilter.c
+++ 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().
+ *
  * Returns the number of network filters found or -1 in case of error
  */
 int
diff --git a/src/libvirt-secret.c b/src/libvirt-secret.c
index 75d40f53dc..c836a09ce7 100644
--- a/src/libvirt-secret.c
+++ b/src/libvirt-secret.c
@@ -156,6 +156,9 @@ virConnectListAllSecrets(virConnectPtr conn,
  *
  * List UUIDs of defined secrets, store pointers to names in uuids.
  *
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllSecrets().
+ *
  * Returns the number of UUIDs provided in the array, or -1 on failure.
  */
 int
diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
index 2a7cdca234..6b2f612cac 100644
--- a/src/libvirt-storage.c
+++ b/src/libvirt-storage.c
@@ -179,7 +179,8 @@ virConnectNumOfStoragePools(virConnectPtr conn)
  * If there are more than maxnames, the remaining names will be silently
  * ignored.
  *
- * For more control over the results, see virConnectListAllStoragePools().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllStoragePools().
  *
  * Returns the number of pools found or -1 in case of error.  Note that
  * this command is inherently racy; a pool can be started between a call to
@@ -259,7 +260,8 @@ virConnectNumOfDefinedStoragePools(virConnectPtr conn)
  * If there are more than maxnames, the remaining names will be silently
  * ignored.
  *
- * For more control over the results, see virConnectListAllStoragePools().
+ * The use of this function in new code is discouraged. Instead, use
+ * virConnectListAllStoragePools().
  *
  * Returns the number of names provided in the array or -1 in case of error.
  * Note that this command is inherently racy; a pool can be defined between
@@ -1254,7 +1256,8 @@ virStoragePoolNumOfVolumes(virStoragePoolPtr pool)
  * Fetch list of storage volume names, limiting to
  * at most maxnames.
  *
- * To list the volume objects directly, see virStoragePoolListAllVolumes().
+ * The use of this function in new code is discouraged. Instead, use
+ * virStoragePoolListAllVolumes().
  *
  * Returns the number of names fetched, or -1 on error
  */
-- 
2.26.2

Re: [libvirt PATCH] API: discourage usage of non-ListAll APIs
Posted by Andrea Bolognani 3 years, 6 months ago
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