[PATCH v2 0/2] Remove @masked argument from virSocketAddrFormatWithPrefix
Since its introduction in commit 426afc00, the function virSocketAddrFormatWithPrefix has never been called with argument masked=false. So, this commit proposes to remove this unnecessary argument from the function scope. This series also proposes the respective test cases for virSocketAddrFormatWithPrefix (now, without @masked argument). Julio Faracco (2): util: Remove @masked argument from virSocketAddrFormatWithPrefix tests: Add tests for virSocketAddrFormatWithPrefix src/network/network_iptables.c | 10 +++--- src/network/network_nftables.c | 10 +++--- src/util/virsocketaddr.c | 14 ++++---- src/util/virsocketaddr.h | 3 +- tests/sockettest.c | 63 ++++++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 20 deletions(-) -- 2.52.0
The function virSocketAddrFormatWithPrefix is being used only in network_nftables.c and network_iptables.c. In both cases, the masked argument is always true. There is no place where this function is being called with masked=false since its introduction in commit 426afc00. In other words, to the current state of libvirt code, we can remove this argument. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/network/network_iptables.c | 10 +++++----- src/network/network_nftables.c | 10 +++++----- src/util/virsocketaddr.c | 14 ++++++-------- src/util/virsocketaddr.h | 3 +-- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/network/network_iptables.c b/src/network/network_iptables.c index d21ce59b70..6002d120b0 100644 --- a/src/network/network_iptables.c +++ b/src/network/network_iptables.c @@ -384,7 +384,7 @@ iptablesForwardAllowOut(virFirewall *fw, virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; if (physdev && physdev[0]) @@ -474,7 +474,7 @@ iptablesForwardAllowRelatedIn(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; g_autofree char *networkstr = NULL; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; if (physdev && physdev[0]) @@ -566,7 +566,7 @@ iptablesForwardAllowIn(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; g_autofree char *networkstr = NULL; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; if (physdev && physdev[0]) @@ -820,7 +820,7 @@ iptablesForwardMasquerade(virFirewall *fw, virFirewallLayer layer = af == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->start, af)) { @@ -965,7 +965,7 @@ iptablesForwardDontMasquerade(virFirewall *fw, virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; if (physdev && physdev[0]) diff --git a/src/network/network_nftables.c b/src/network/network_nftables.c index 5d716264bf..cffe8072f5 100644 --- a/src/network/network_nftables.c +++ b/src/network/network_nftables.c @@ -354,7 +354,7 @@ nftablesAddForwardAllowOut(virFirewall *fw, const char *layerStr = nftablesLayerTypeToString(layer); virFirewallCmd *fwCmd; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; fwCmd = virFirewallAddCmd(fw, layer, "insert", "rule", @@ -392,7 +392,7 @@ nftablesAddForwardAllowRelatedIn(virFirewall *fw, g_autofree char *networkstr = NULL; virFirewallCmd *fwCmd; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; fwCmd = virFirewallAddCmd(fw, layer, "insert", "rule", @@ -430,7 +430,7 @@ nftablesAddForwardAllowIn(virFirewall *fw, g_autofree char *networkstr = NULL; virFirewallCmd *fwCmd; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; fwCmd = virFirewallAddCmd(fw, layer, "insert", "rule", @@ -544,7 +544,7 @@ nftablesAddForwardMasquerade(virFirewall *fw, VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; const char *layerStr = nftablesLayerTypeToString(layer); - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->start, af)) { @@ -628,7 +628,7 @@ nftablesAddDontMasquerade(virFirewall *fw, const char *layerStr = nftablesLayerTypeToString(layer); virFirewallCmd *fwCmd; - if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix, true))) + if (!(networkstr = virSocketAddrFormatWithPrefix(netaddr, prefix))) return -1; fwCmd = virFirewallAddCmd(fw, layer, "insert", "rule", diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 4d4a6b2a0f..1f203fb50d 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -529,17 +529,15 @@ virSocketAddrFormatFull(const virSocketAddr *addr, * @masked: true to mask off the host bits of the address * * Returns a string representation of the IP network described by - * @netaddr/@prefix. If @masked is true, the address is masked to - * remove the host bits according to prefix. So, for example, sending - * f(1.2.3.4, 24, true) would return "1.2.3.0/24", but f(1.2.3.4, 24, - * false) would return "1.2.3.4/24". + * @addr/@prefix. The address is masked to remove the host bits + * according to prefix. So, for example, sending + * f(1.2.3.4, 24) would return "1.2.3.0/24". * - * returns false on failure (and logs an error message) + * Returns NULL on failure (and logs an error message) */ char * virSocketAddrFormatWithPrefix(virSocketAddr *addr, - unsigned int prefix, - bool masked) + unsigned int prefix) { virSocketAddr network; g_autofree char *netstr = NULL; @@ -551,7 +549,7 @@ virSocketAddrFormatWithPrefix(virSocketAddr *addr, return NULL; } - if (masked && virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { + if (virSocketAddrMaskByPrefix(addr, prefix, &network) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failure to mask address")); return NULL; diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 47b8effa85..c7ad3250e0 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -89,8 +89,7 @@ char *virSocketAddrFormatFull(const virSocketAddr *addr, bool withService, const char *separator); char *virSocketAddrFormatWithPrefix(virSocketAddr *addr, - unsigned int prefix, - bool masked); + unsigned int prefix); char *virSocketAddrGetPath(virSocketAddr *addr); -- 2.52.0
On a Monday in 2026, Julio Faracco wrote:
The function virSocketAddrFormatWithPrefix is being used only in network_nftables.c and network_iptables.c. In both cases, the masked argument is always true. There is no place where this function is being called with masked=false since its introduction in commit 426afc00. In other words, to the current state of libvirt code, we can remove this argument.
To remove the period after the commit message and the redundant sentences, I simplified the commit message to: Ever since its introduction in commit 426afc00 all the callers pass true. Remove the argument.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/network/network_iptables.c | 10 +++++----- src/network/network_nftables.c | 10 +++++----- src/util/virsocketaddr.c | 14 ++++++-------- src/util/virsocketaddr.h | 3 +-- 4 files changed, 17 insertions(+), 20 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Add comprehensive test coverage for virSocketAddrFormatWithPrefix() to verify its behavior by adding macros DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX and DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX and the respective testing function testFormatWithPrefixHelper. This commit also adds some error handling for the unsupported AF_UNIX family. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tests/sockettest.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/sockettest.c b/tests/sockettest.c index 5cb8a9fb72..9e185cc234 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -55,6 +55,22 @@ static int testFormat(virSocketAddr *addr, const char *addrstr, bool pass) } } +static int testFormatWithPrefix(virSocketAddr *addr, const char *addrstr, + unsigned int prefix, bool pass) +{ + g_autofree char *newaddrstr = NULL; + + newaddrstr = virSocketAddrFormatWithPrefix(addr, prefix); + if (!newaddrstr) + return pass ? -1 : 0; + + if (virTestCompareToString(newaddrstr, addrstr) < 0) { + return pass ? -1 : 0; + } else { + return pass ? 0 : -1; + } +} + struct testParseData { virSocketAddr *addr; const char *addrstr; @@ -78,6 +94,19 @@ static int testFormatHelper(const void *opaque) return testFormat(data->addr, data->addrstr, data->pass); } +struct testFormatWithPrefixData { + virSocketAddr *addr; + const char *addrstr; + unsigned int prefix; + bool pass; +}; +static int testFormatWithPrefixHelper(const void *opaque) +{ + const struct testFormatWithPrefixData *data = opaque; + return testFormatWithPrefix(data->addr, data->addrstr, data->prefix, + data->pass); +} + static int testRange(const char *saddrstr, const char *eaddrstr, @@ -293,6 +322,32 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX(addrstr, family, prefix, pass) \ + do { \ + virSocketAddr addr = { 0 }; \ + struct testParseData data = { &addr, addrstr, family, pass }; \ + struct testFormatWithPrefixData data2 = { &addr, addrstr, prefix, pass }; \ + if (virTestRun("Test parse " addrstr " family " #family, \ + testParseHelper, &data) < 0) \ + ret = -1; \ + if (virTestRun("Test format " addrstr " family " #family " with prefix /" #prefix, \ + testFormatWithPrefixHelper, &data2) < 0) \ + ret = -1; \ + } while (0) + +#define DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX(addrstr, addrformated, family, prefix, pass) \ + do { \ + virSocketAddr addr = { 0 }; \ + struct testParseData data = { &addr, addrstr, family, pass }; \ + struct testFormatWithPrefixData data2 = { &addr, addrformated, prefix, pass }; \ + if (virTestRun("Test parse " addrstr " family " #family, \ + testParseHelper, &data) < 0) \ + ret = -1; \ + if (virTestRun("Test format " addrstr " family " #family " with prefix /" #prefix, \ + testFormatWithPrefixHelper, &data2) < 0) \ + ret = -1; \ + } while (0) + #define DO_TEST_RANGE(saddr, eaddr, netaddr, prefix, size, pass) \ do { \ struct testRangeData data \ @@ -376,6 +431,14 @@ mymain(void) DO_TEST_PARSE_AND_FORMAT("::fffe:0:0", AF_UNSPEC, true); DO_TEST_PARSE_AND_FORMAT("::ffff:10.1.2.3", AF_UNSPEC, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("1.2.3.4", "1.2.3.0/24", AF_INET, 24, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("10.1.1.12", "10.0.0.0/8", AF_INET, 8, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("192.168.1.124", "192.168.0.0/16", AF_INET, 16, true); + DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX("2001:db8:dead:beef:1::", "2001:db8:dead:beef::/64", AF_INET6, 64, true); + + DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX("1.2.3.4", AF_UNIX, 24, false); + DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX("2001:db8:dead:beef:1::", AF_UNIX, 64, false); + /* tests that specify a network that should contain the range */ DO_TEST_RANGE("192.168.122.1", "192.168.122.1", "192.168.122.1", 24, 1, true); DO_TEST_RANGE("192.168.122.1", "192.168.122.20", "192.168.122.22", 24, 20, true); -- 2.52.0
On a Monday in 2026, Julio Faracco wrote:
Add comprehensive test coverage for virSocketAddrFormatWithPrefix()
Calling it comprehensive is quite an exaggeration, since it has no negative tests for IP addresses.
to verify its behavior by adding macros DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX and DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX and the respective
No need to name the long macros here, the reader can see them in the commit itself.
testing function testFormatWithPrefixHelper.
The function name is self-evident.
This commit also adds some error handling for the unsupported AF_UNIX family.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tests/sockettest.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)
diff --git a/tests/sockettest.c b/tests/sockettest.c index 5cb8a9fb72..9e185cc234 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -55,6 +55,22 @@ static int testFormat(virSocketAddr *addr, const char *addrstr, bool pass) } }
+static int testFormatWithPrefix(virSocketAddr *addr, const char *addrstr, + unsigned int prefix, bool pass)
There's a TAB in the indentation here that makes the syntax check fail.
+{ + g_autofree char *newaddrstr = NULL; + + newaddrstr = virSocketAddrFormatWithPrefix(addr, prefix); + if (!newaddrstr) + return pass ? -1 : 0; + + if (virTestCompareToString(newaddrstr, addrstr) < 0) { + return pass ? -1 : 0; + } else { + return pass ? 0 : -1;
This needs at least a VIR_TEST_DEBUG here, since virTestCompareToString does not log anything in case of success Jano
+ } +} + struct testParseData { virSocketAddr *addr; const char *addrstr;
Em qua., 21 de jan. de 2026 às 12:57, Ján Tomko <jtomko@redhat.com> escreveu:
On a Monday in 2026, Julio Faracco wrote:
Add comprehensive test coverage for virSocketAddrFormatWithPrefix()
Calling it comprehensive is quite an exaggeration, since it has no negative tests for IP addresses.
to verify its behavior by adding macros DO_TEST_PARSE_AND_FORMAT_WITH_PREFIX and DO_TEST_PARSE_AND_CHECK_FORMAT_WITH_PREFIX and the respective
No need to name the long macros here, the reader can see them in the commit itself.
testing function testFormatWithPrefixHelper.
The function name is self-evident.
This commit also adds some error handling for the unsupported AF_UNIX family.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- tests/sockettest.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)
diff --git a/tests/sockettest.c b/tests/sockettest.c index 5cb8a9fb72..9e185cc234 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -55,6 +55,22 @@ static int testFormat(virSocketAddr *addr, const char *addrstr, bool pass) } }
+static int testFormatWithPrefix(virSocketAddr *addr, const char *addrstr, + unsigned int prefix, bool pass)
There's a TAB in the indentation here that makes the syntax check fail.
+{ + g_autofree char *newaddrstr = NULL; + + newaddrstr = virSocketAddrFormatWithPrefix(addr, prefix); + if (!newaddrstr) + return pass ? -1 : 0; + + if (virTestCompareToString(newaddrstr, addrstr) < 0) { + return pass ? -1 : 0; + } else { + return pass ? 0 : -1;
This needs at least a VIR_TEST_DEBUG here, since virTestCompareToString does not log anything in case of success
I think I will include VIR_TEST_DEBUG for both cases (success and failure). Sometimes you are expecting to test a failure as the argument @pass suggests. Let me create a third patch, because testFormat() has a similar behavior.
Jano
+ } +} + struct testParseData { virSocketAddr *addr; const char *addrstr;
-- Julio
participants (2)
-
Julio Faracco -
Ján Tomko