[PATCH] util: netdevvlan: Change return type of virNetDevVlanCopy to void
This function return value is invariant since 1022e0ee, so change its type and remove all dependent checks. Found by Linux Verification Center (linuxtesting.org) with Svace. Reported-by: Alexander Rudyuk <a.rudyuk@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org> --- src/conf/domain_conf.c | 9 +++------ src/network/bridge_driver.c | 4 +++- src/util/virnetdevvlan.c | 6 +++--- src/util/virnetdevvlan.h | 2 +- 4 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3f88a77a8f..6f49ee507a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30533,8 +30533,7 @@ virDomainNetDefToNetworkPort(virDomainDef *dom, if (virNetDevBandwidthCopy(&port->bandwidth, iface->bandwidth) < 0) return NULL; - if (virNetDevVlanCopy(&port->vlan, &iface->vlan) < 0) - return NULL; + virNetDevVlanCopy(&port->vlan, &iface->vlan); port->isolatedPort = iface->isolatedPort; port->trustGuestRxFilters = iface->trustGuestRxFilters; @@ -30611,8 +30610,7 @@ virDomainNetDefActualFromNetworkPort(virDomainNetDef *iface, if (virNetDevBandwidthCopy(&actual->bandwidth, port->bandwidth) < 0) goto error; - if (virNetDevVlanCopy(&actual->vlan, &port->vlan) < 0) - goto error; + virNetDevVlanCopy(&actual->vlan, &port->vlan); actual->isolatedPort = port->isolatedPort; actual->class_id = port->class_id; @@ -30729,8 +30727,7 @@ virDomainNetDefActualToNetworkPort(virDomainDef *dom, if (virNetDevBandwidthCopy(&port->bandwidth, actual->bandwidth) < 0) return NULL; - if (virNetDevVlanCopy(&port->vlan, &actual->vlan) < 0) - return NULL; + virNetDevVlanCopy(&port->vlan, &actual->vlan); port->isolatedPort = actual->isolatedPort; port->class_id = actual->class_id; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8f47ef2574..668870a9ee 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3973,8 +3973,10 @@ networkAllocatePort(virNetworkObj *obj, else if (netdef->vlan.nTags > 0) vlan = &netdef->vlan; - if (vlan && virNetDevVlanCopy(&port->vlan, vlan) < 0) + if (vlan) { + virNetDevVlanCopy(&port->vlan, vlan); return -1; + } } if (!port->trustGuestRxFilters) { diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c index 67daa5d3b4..e8f572efd2 100644 --- a/src/util/virnetdevvlan.c +++ b/src/util/virnetdevvlan.c @@ -76,11 +76,11 @@ virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b) * If src is NULL, dst will have nTags set to 0. * dst is assumed to be empty on entry. */ -int +void virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src) { if (!src || src->nTags == 0) - return 0; + return; dst->tag = g_new0(unsigned int, src->nTags); dst->trunk = src->trunk; @@ -88,5 +88,5 @@ virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src) dst->nativeMode = src->nativeMode; dst->nativeTag = src->nativeTag; memcpy(dst->tag, src->tag, src->nTags * sizeof(*src->tag)); - return 0; + return; } diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h index 228d270869..fd2f8023f5 100644 --- a/src/util/virnetdevvlan.h +++ b/src/util/virnetdevvlan.h @@ -42,6 +42,6 @@ struct _virNetDevVlan { void virNetDevVlanClear(virNetDevVlan *vlan); void virNetDevVlanFree(virNetDevVlan *vlan); int virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b); -int virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src); +void virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevVlan, virNetDevVlanFree); -- 2.42.4
Kindly reminding about this small cosmetic fix
On Wed, Jan 22, 2025 at 05:37:31PM +0300, Alexander Kuznetsov wrote:
This function return value is invariant since 1022e0ee, so change its type and remove all dependent checks.
Found by Linux Verification Center (linuxtesting.org) with Svace.
Reported-by: Alexander Rudyuk <a.rudyuk@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org> --- src/conf/domain_conf.c | 9 +++------ src/network/bridge_driver.c | 4 +++- src/util/virnetdevvlan.c | 6 +++--- src/util/virnetdevvlan.h | 2 +- 4 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8f47ef2574..668870a9ee 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3973,8 +3973,10 @@ networkAllocatePort(virNetworkObj *obj, else if (netdef->vlan.nTags > 0) vlan = &netdef->vlan;
- if (vlan && virNetDevVlanCopy(&port->vlan, vlan) < 0) + if (vlan) { + virNetDevVlanCopy(&port->vlan, vlan); return -1; + }
This is *definitely* not semantically the same as before, beware of such changes!
diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c index 67daa5d3b4..e8f572efd2 100644 --- a/src/util/virnetdevvlan.c +++ b/src/util/virnetdevvlan.c @@ -76,11 +76,11 @@ virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b) * If src is NULL, dst will have nTags set to 0. * dst is assumed to be empty on entry. */ -int +void virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src) { if (!src || src->nTags == 0) - return 0; + return;
This means you don't even need to check if the source vlan is not NULL before calling this function.
dst->tag = g_new0(unsigned int, src->nTags); dst->trunk = src->trunk; @@ -88,5 +88,5 @@ virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src) dst->nativeMode = src->nativeMode; dst->nativeTag = src->nativeTag; memcpy(dst->tag, src->tag, src->nTags * sizeof(*src->tag)); - return 0; + return;
Pointless return at the end of a function
}
Thanks for the review! v2: - remove check for source vlan being not NULL and fix wrongly changed statement - remove pointless return at the end of the function Alexander Kuznetsov (1): util: netdevvlan: Change return type of virNetDevVlanCopy to void src/conf/domain_conf.c | 9 +++------ src/network/bridge_driver.c | 3 +-- src/util/virnetdevvlan.c | 5 ++--- src/util/virnetdevvlan.h | 2 +- 4 files changed, 7 insertions(+), 12 deletions(-) -- 2.42.4
This function return value is invariant since 1022e0ee, so change its type and remove all dependent checks. Found by Linux Verification Center (linuxtesting.org) with Svace. Reported-by: Alexander Rudyuk <a.rudyuk@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org> --- src/conf/domain_conf.c | 9 +++------ src/network/bridge_driver.c | 3 +-- src/util/virnetdevvlan.c | 5 ++--- src/util/virnetdevvlan.h | 2 +- 4 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f42b7075ad..032e2f9a6d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30501,8 +30501,7 @@ virDomainNetDefToNetworkPort(virDomainDef *dom, if (virNetDevBandwidthCopy(&port->bandwidth, iface->bandwidth) < 0) return NULL; - if (virNetDevVlanCopy(&port->vlan, &iface->vlan) < 0) - return NULL; + virNetDevVlanCopy(&port->vlan, &iface->vlan); port->isolatedPort = iface->isolatedPort; port->trustGuestRxFilters = iface->trustGuestRxFilters; @@ -30579,8 +30578,7 @@ virDomainNetDefActualFromNetworkPort(virDomainNetDef *iface, if (virNetDevBandwidthCopy(&actual->bandwidth, port->bandwidth) < 0) goto error; - if (virNetDevVlanCopy(&actual->vlan, &port->vlan) < 0) - goto error; + virNetDevVlanCopy(&actual->vlan, &port->vlan); actual->isolatedPort = port->isolatedPort; actual->class_id = port->class_id; @@ -30697,8 +30695,7 @@ virDomainNetDefActualToNetworkPort(virDomainDef *dom, if (virNetDevBandwidthCopy(&port->bandwidth, actual->bandwidth) < 0) return NULL; - if (virNetDevVlanCopy(&port->vlan, &actual->vlan) < 0) - return NULL; + virNetDevVlanCopy(&port->vlan, &actual->vlan); port->isolatedPort = actual->isolatedPort; port->class_id = actual->class_id; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8f47ef2574..80d2c3a1d5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3973,8 +3973,7 @@ networkAllocatePort(virNetworkObj *obj, else if (netdef->vlan.nTags > 0) vlan = &netdef->vlan; - if (vlan && virNetDevVlanCopy(&port->vlan, vlan) < 0) - return -1; + virNetDevVlanCopy(&port->vlan, vlan); } if (!port->trustGuestRxFilters) { diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c index 67daa5d3b4..b0e05d8ffe 100644 --- a/src/util/virnetdevvlan.c +++ b/src/util/virnetdevvlan.c @@ -76,11 +76,11 @@ virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b) * If src is NULL, dst will have nTags set to 0. * dst is assumed to be empty on entry. */ -int +void virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src) { if (!src || src->nTags == 0) - return 0; + return; dst->tag = g_new0(unsigned int, src->nTags); dst->trunk = src->trunk; @@ -88,5 +88,4 @@ virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src) dst->nativeMode = src->nativeMode; dst->nativeTag = src->nativeTag; memcpy(dst->tag, src->tag, src->nTags * sizeof(*src->tag)); - return 0; } diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h index 228d270869..fd2f8023f5 100644 --- a/src/util/virnetdevvlan.h +++ b/src/util/virnetdevvlan.h @@ -42,6 +42,6 @@ struct _virNetDevVlan { void virNetDevVlanClear(virNetDevVlan *vlan); void virNetDevVlanFree(virNetDevVlan *vlan); int virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b); -int virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src); +void virNetDevVlanCopy(virNetDevVlan *dst, const virNetDevVlan *src); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevVlan, virNetDevVlanFree); -- 2.42.4
On Fri, Mar 07, 2025 at 12:08:03PM +0300, Alexander Kuznetsov wrote:
This function return value is invariant since 1022e0ee, so change its type and remove all dependent checks.
Found by Linux Verification Center (linuxtesting.org) with Svace.
Reported-by: Alexander Rudyuk <a.rudyuk@fobos-nt.ru> Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
participants (2)
-
Alexander Kuznetsov -
Martin Kletzander