Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels

2020-07-21 Thread Laine Stump 

On 7/21/20 8:04 AM, John Ferlan wrote:



On 7/7/20 5:08 PM, Laine Stump wrote:

All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.

Signed-off-by: Laine Stump 
---
  src/network/bridge_driver.c   | 264 --
  src/network/bridge_driver_linux.c |  15 +-
  2 files changed, 113 insertions(+), 166 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c


[...]

Coverity noted there's a leak with this part of the change for @field...

  
@@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)

  {
  virNetworkDefPtr def = virNetworkObjGetDef(obj);
  g_autofree char *field = NULL;
-int ret = -1;
  bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
  
  /* set disable_ipv6 if there are no ipv6 addresses defined for the

@@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
  if (!enableIPv6)
  VIR_DEBUG("ipv6 appears to already be disabled on %s",
def->bridge);
-ret = 0;
-goto cleanup;
+return 0;
  }


Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.


Sigh.

I *thought* I had removed all the changes to this function when I 
rebased the series the last time (since Jan already had a better patch 
for it), but I guess I didn't look carefully enough at the diffs before 
I pushed :-(


Fortunately, Jan has pushed his patch, which completely replaces the 
function.





  
  if (virFileWriteStr(field, enableIPv6 ? "0" : "1", 0) < 0) {

  virReportSystemError(errno,
   _("cannot write to %s to enable/disable IPv6 "
 "on bridge %s"), field, def->bridge);
-goto cleanup;
+return -1;
  }
  
  /* The rest of the ipv6 sysctl tunables should always be set the

@@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
  if (virFileWriteStr(field, "0", 0) < 0) {
  virReportSystemError(errno,
   _("cannot disable %s"), field);
-goto cleanup;
+return -1;
  }
  
  /* All interfaces used as a gateway (which is what this is, by

@@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
  if (virFileWriteStr(field, "0", 0) < 0) {
  virReportSystemError(errno,
   _("cannot disable %s"), field);
-goto cleanup;
+return -1;
  }
  
-ret = 0;

- cleanup:
-return ret;
+return 0;
  }


[...]

Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels

2020-07-21 Thread Ján Tomko 

On a Tuesday in 2020, John Ferlan wrote:



On 7/7/20 5:08 PM, Laine Stump wrote:

All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c   | 264 --
 src/network/bridge_driver_linux.c |  15 +-
 2 files changed, 113 insertions(+), 166 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c


[...]

Coverity noted there's a leak with this part of the change for @field...



@@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
 {
 virNetworkDefPtr def = virNetworkObjGetDef(obj);
 g_autofree char *field = NULL;
-int ret = -1;
 bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);

 /* set disable_ipv6 if there are no ipv6 addresses defined for the
@@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
 if (!enableIPv6)
 VIR_DEBUG("ipv6 appears to already be disabled on %s",
   def->bridge);
-ret = 0;
-goto cleanup;
+return 0;
 }


Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.



Should be fixed in git master by:
commit 5c50d1dda5664d480e6370111c0218947706bd31
Author: Ján Tomko 
CommitDate: 2020-07-21 14:55:00 +0200

network: split out networkSetIPv6Sysctl

https://gitlab.com/libvirt/libvirt/-/commit/5c50d1dda5664d480e6370111c0218947706bd31

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels

2020-07-21 Thread John Ferlan 



On 7/7/20 5:08 PM, Laine Stump wrote:
> All these cleanup/error labels were reduced to having just "return
> ret" by a previous patch, so get rid of them and return directly.
> 
> Signed-off-by: Laine Stump 
> ---
>  src/network/bridge_driver.c   | 264 --
>  src/network/bridge_driver_linux.c |  15 +-
>  2 files changed, 113 insertions(+), 166 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 31bd0dd92c..79b2ca3330 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c

[...]

Coverity noted there's a leak with this part of the change for @field...

>  
> @@ -2207,7 +2164,6 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>  {
>  virNetworkDefPtr def = virNetworkObjGetDef(obj);
>  g_autofree char *field = NULL;
> -int ret = -1;
>  bool enableIPv6 = !!virNetworkDefGetIPByIndex(def, AF_INET6, 0);
>  
>  /* set disable_ipv6 if there are no ipv6 addresses defined for the
> @@ -2221,15 +2177,14 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>  if (!enableIPv6)
>  VIR_DEBUG("ipv6 appears to already be disabled on %s",
>def->bridge);
> -ret = 0;
> -goto cleanup;
> +return 0;
>  }

Below here doesn't match w/ current source, but I assume you know that.
Looks like a mis-merge with the review from the previous patch.

>  
>  if (virFileWriteStr(field, enableIPv6 ? "0" : "1", 0) < 0) {
>  virReportSystemError(errno,
>   _("cannot write to %s to enable/disable IPv6 "
> "on bridge %s"), field, def->bridge);
> -goto cleanup;
> +return -1;
>  }
>  
>  /* The rest of the ipv6 sysctl tunables should always be set the
> @@ -2246,7 +2201,7 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>  if (virFileWriteStr(field, "0", 0) < 0) {
>  virReportSystemError(errno,
>   _("cannot disable %s"), field);
> -goto cleanup;
> +return -1;
>  }
>  
>  /* All interfaces used as a gateway (which is what this is, by
> @@ -2258,12 +2213,10 @@ networkSetIPv6Sysctls(virNetworkObjPtr obj)
>  if (virFileWriteStr(field, "0", 0) < 0) {
>  virReportSystemError(errno,
>   _("cannot disable %s"), field);
> -goto cleanup;
> +return -1;
>  }
>  
> -ret = 0;
> - cleanup:
> -return ret;
> +return 0;
>  }

[...]

Re: [libvirt PATCH v2 06/15] network: eliminate unnecessary labels

2020-07-15 Thread Ján Tomko 

On a Tuesday in 2020, Laine Stump wrote:

All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.

Signed-off-by: Laine Stump 
---
src/network/bridge_driver.c   | 264 --
src/network/bridge_driver_linux.c |  15 +-
2 files changed, 113 insertions(+), 166 deletions(-)

@@ -3190,7 +3143,7 @@ static int
networkFindUnusedBridgeName(virNetworkObjListPtr nets,
virNetworkDefPtr def)
{
-int ret = -1, id = 0;
+int id = 0;
const char *templ = "virbr%d";
const char *p;

@@ -3211,17 +3164,14 @@ networkFindUnusedBridgeName(virNetworkObjListPtr nets,
  virNetDevExists(newname) == 1)) {
VIR_FREE(def->bridge); /*could contain template */
def->bridge = g_steal_pointer();
-ret = 0;
-goto cleanup;
+return 0;
}
} while (++id <= MAX_BRIDGE_ID);

virReportError(VIR_ERR_INTERNAL_ERROR,
   _("Bridge generation exceeded max id %d"),
   MAX_BRIDGE_ID);
-ret = 0;
- cleanup:
-return ret;
+return -1;


This fix deserves a separate commit. Or at least a mention in the commit
message.


}




Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

[libvirt PATCH v2 06/15] network: eliminate unnecessary labels

2020-07-07 Thread Laine Stump 
All these cleanup/error labels were reduced to having just "return
ret" by a previous patch, so get rid of them and return directly.

Signed-off-by: Laine Stump 
---
 src/network/bridge_driver.c   | 264 --
 src/network/bridge_driver_linux.c |  15 +-
 2 files changed, 113 insertions(+), 166 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 31bd0dd92c..79b2ca3330 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -197,18 +197,14 @@ networkDnsmasqDefNamespaceParse(xmlXPathContextPtr ctxt,
 void **data)
 {
 g_autoptr(networkDnsmasqXmlNsDef) nsdata = g_new0(networkDnsmasqXmlNsDef, 
1);
-int ret = -1;
 
 if (networkDnsmasqDefNamespaceParseOptions(nsdata, ctxt))
-goto cleanup;
+return -1;
 
 if (nsdata->noptions > 0)
 *data = g_steal_pointer();
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -331,22 +327,20 @@ networkRunHook(virNetworkObjPtr obj,
 g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
 g_autofree char *xml = NULL;
 int hookret;
-int ret = -1;
 
 if (virHookPresent(VIR_HOOK_DRIVER_NETWORK)) {
 if (!obj) {
 VIR_DEBUG("Not running hook as @obj is NULL");
-ret = 0;
-goto cleanup;
+return 0;
 }
 def = virNetworkObjGetDef(obj);
 
 virBufferAddLit(, "\n");
 virBufferAdjustIndent(, 2);
 if (virNetworkDefFormatBuf(, def, network_driver->xmlopt, 0) < 0)
-goto cleanup;
+return -1;
 if (port && virNetworkPortDefFormatBuf(, port) < 0)
-goto cleanup;
+return -1;
 
 virBufferAdjustIndent(, -2);
 virBufferAddLit(, "");
@@ -359,14 +353,12 @@ networkRunHook(virNetworkObjPtr obj,
  * If the script raised an error, pass it to the callee.
  */
 if (hookret < 0)
-goto cleanup;
+return -1;
 
 networkNetworkObjTaint(obj, VIR_NETWORK_TAINT_HOOK);
 }
 
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -440,34 +432,32 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
 g_autoptr(dnsmasqContext) dctx = NULL;
 virNetworkDefPtr def = virNetworkObjGetPersistentDef(obj);
 
-int ret = -1;
-
 /* remove the (possibly) existing dnsmasq and radvd files */
 if (!(dctx = dnsmasqContextNew(def->name,
driver->dnsmasqStateDir))) {
-goto cleanup;
+return -1;
 }
 
 if (!(leasefile = networkDnsmasqLeaseFileNameDefault(driver, def->name)))
-goto cleanup;
+return -1;
 
 if (!(customleasefile = networkDnsmasqLeaseFileNameCustom(driver, 
def->bridge)))
-goto cleanup;
+return -1;
 
 if (!(radvdconfigfile = networkRadvdConfigFileName(driver, def->name)))
-goto cleanup;
+return -1;
 
 if (!(radvdpidbase = networkRadvdPidfileBasename(def->name)))
-goto cleanup;
+return -1;
 
 if (!(configfile = networkDnsmasqConfigFileName(driver, def->name)))
-goto cleanup;
+return -1;
 
 if (!(statusfile = virNetworkConfigFile(driver->stateDir, def->name)))
-goto cleanup;
+return -1;
 
 if (!(macMapFile = virMacMapFileName(driver->dnsmasqStateDir, 
def->bridge)))
-goto cleanup;
+return -1;
 
 /* dnsmasq */
 dnsmasqDelete(dctx);
@@ -488,10 +478,7 @@ networkRemoveInactive(virNetworkDriverStatePtr driver,
 /* remove the network definition */
 virNetworkObjRemoveInactive(driver->networks, obj);
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -703,7 +690,6 @@ networkStateInitialize(bool privileged,
virStateInhibitCallback callback G_GNUC_UNUSED,
void *opaque G_GNUC_UNUSED)
 {
-int ret = VIR_DRV_STATE_INIT_ERROR;
 g_autofree char *configdir = NULL;
 g_autofree char *rundir = NULL;
 bool autostart = true;
@@ -831,13 +817,12 @@ networkStateInitialize(bool privileged,
 }
 #endif
 
-ret = VIR_DRV_STATE_INIT_COMPLETE;
- cleanup:
-return ret;
+return VIR_DRV_STATE_INIT_COMPLETE;
+
 
  error:
 networkStateCleanup();
-goto cleanup;
+return VIR_DRV_STATE_INIT_ERROR;
 }
 
 
@@ -1074,7 +1059,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
 {
 virNetworkDefPtr def = virNetworkObjGetDef(obj);
 g_auto(virBuffer) configbuf = VIR_BUFFER_INITIALIZER;
-int r, ret = -1;
+int r;
 int nbleases = 0;
 size_t i;
 virNetworkDNSDefPtr dns = >dns;
@@ -1138,7 +1123,7 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
 g_autofree char *addr = virSocketAddrFormat(>addr);
 
 if (!addr)
-goto cleanup;
+return -1;
 virBufferAsprintf(, "%s\n", addr);
 if (!fwd->domain)