Re: [PATCH v4 2/7] parallels: fix parallelsLoadNetworks

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 18.03.2015 09:33, Maxim Nestratov wrote:
> Don't fail initialization of parallels driver if
> parallelsLoadNetwork fails for optional networks.
> This can happen when some of them are added manually
> and configured incompletely. PCS requires only two networks
> created automatically (named Host-Only and Bridged), others
> are optional and their incompletenes can be ignored.

s/incompletenes/incompleteness/

> 
> Signed-off-by: Maxim Nestratov <mnestratov@xxxxxxxxxxxxx>
> ---
>  src/parallels/parallels_network.c |   43 +++++++++++++++++++++++-------------
>  1 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
> index bb7ec5e..1d0ee1c 100644
> --- a/src/parallels/parallels_network.c
> +++ b/src/parallels/parallels_network.c
> @@ -180,9 +180,10 @@ static int parallelsGetHostOnlyNetInfo(virNetworkDefPtr def, const char *name)
>      return ret;
>  }
>  
> -static virNetworkObjPtr
> +static int
>  parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
>  {
> +    int ret = -1;
>      virNetworkObjPtr net;
>      virNetworkDefPtr def;
>      const char *tmp;
> @@ -214,13 +215,25 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
>      if (STREQ(tmp, PARALLELS_BRIDGED_NETWORK_TYPE)) {
>          def->forward.type = VIR_NETWORK_FORWARD_BRIDGE;
>  
> -        if (parallelsGetBridgedNetInfo(def, jobj) < 0)
> +        if (parallelsGetBridgedNetInfo(def, jobj) < 0) {
> +
> +            /* Only mandatory networks are required to be configured completely */
> +            if (STRNEQ(def->name, PARALLELS_REQUIRED_BRIDGED_NETWORK))
> +                ret = 0;
> +
>              goto cleanup;
> +        }
>      } else if (STREQ(tmp, PARALLELS_HOSTONLY_NETWORK_TYPE)) {
>          def->forward.type = VIR_NETWORK_FORWARD_NONE;
>  
> -        if (parallelsGetHostOnlyNetInfo(def, def->name) < 0)
> +        if (parallelsGetHostOnlyNetInfo(def, def->name) < 0) {
> +
> +            /* Only mandatory networks are required to be configured completely */
> +            if (STRNEQ(def->name, PARALLELS_REQUIRED_HOSTONLY_NETWORK))
> +                ret = 0;
> +
>              goto cleanup;
> +        }
>      } else {
>          parallelsParseError();
>          goto cleanup;
> @@ -230,14 +243,16 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
>          goto cleanup;
>      net->active = 1;
>      net->autostart = 1;
> -    return net;
> +    virNetworkObjEndAPI(&net);
> +    ret = 0;
> +    def = NULL;

So if everything goes well, zero is returned ...

>  
>   cleanup:
>      virNetworkDefFree(def);
> -    return NULL;
> +    return ret;
>  }
>  
> -static virNetworkObjPtr
> +static int
>  parallelsAddRoutedNetwork(parallelsConnPtr privconn)
>  {
>      virNetworkObjPtr net = NULL;
> @@ -264,18 +279,18 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
>      }
>      net->active = 1;
>      net->autostart = 1;
> +    virNetworkObjEndAPI(&net);
>  
> -    return net;
> +    return 0;
>  
>   cleanup:
>      virNetworkDefFree(def);

OUCH! Not to be seen in this context, but if virNetworkAssignDef() called a few lines above fails, it'll call virNetworkDefFree() and goto 'cleanup' subsequently where @def is freed the second time.

> -    return NULL;
> +    return -1;
>  }
>  
>  static int parallelsLoadNetworks(parallelsConnPtr privconn)
>  {
>      virJSONValuePtr jobj, jobj2;
> -    virNetworkObjPtr net = NULL;
>      int ret = -1;
>      int count;
>      size_t i;
> @@ -300,22 +315,18 @@ static int parallelsLoadNetworks(parallelsConnPtr privconn)
>              goto cleanup;
>          }
>  
> -        net = parallelsLoadNetwork(privconn, jobj2);
> -        if (!net)
> +        ret = parallelsLoadNetwork(privconn, jobj2);
> +        if (!ret)

... so this should have been (ret) instead of (!ret).

>              goto cleanup;
> -        else
> -            virNetworkObjEndAPI(&net);
> -
>      }
>  
> -    if (!(net = parallelsAddRoutedNetwork(privconn)))
> +    if (!(ret = parallelsAddRoutedNetwork(privconn)))

Again, parallelsAddRoutedNetwork() returns zero on success.

>          goto cleanup;
>  
>      ret = 0;
>  
>   cleanup:
>      virJSONValueFree(jobj);
> -    virNetworkObjEndAPI(&net);
>      return ret;
>  }
>  
> 

I'm squashing this in:

diff --git a/src/parallels/parallels_network.c b/src/parallels/parallels_network.c
index 1d0ee1c..8cc0582 100644
--- a/src/parallels/parallels_network.c
+++ b/src/parallels/parallels_network.c
@@ -184,7 +184,7 @@ static int
 parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
 {
     int ret = -1;
-    virNetworkObjPtr net;
+    virNetworkObjPtr net = NULL;
     virNetworkDefPtr def;
     const char *tmp;
     /* MD5_DIGEST_SIZE = VIR_UUID_BUFLEN = 16 */
@@ -241,13 +241,13 @@ parallelsLoadNetwork(parallelsConnPtr privconn, virJSONValuePtr jobj)
 
     if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
         goto cleanup;
+    def = NULL;
     net->active = 1;
     net->autostart = 1;
-    virNetworkObjEndAPI(&net);
     ret = 0;
-    def = NULL;
 
  cleanup:
+    virNetworkObjEndAPI(&net);
     virNetworkDefFree(def);
     return ret;
 }
@@ -273,10 +273,9 @@ parallelsAddRoutedNetwork(parallelsConnPtr privconn)
     }
     def->uuid_specified = 1;
 
-    if (!(net = virNetworkAssignDef(privconn->networks, def, false))) {
-        virNetworkDefFree(def);
+    if (!(net = virNetworkAssignDef(privconn->networks, def, false)))
         goto cleanup;
-    }
+
     net->active = 1;
     net->autostart = 1;
     virNetworkObjEndAPI(&net);
@@ -315,12 +314,11 @@ static int parallelsLoadNetworks(parallelsConnPtr privconn)
             goto cleanup;
         }
 
-        ret = parallelsLoadNetwork(privconn, jobj2);
-        if (!ret)
+        if (parallelsLoadNetwork(privconn, jobj2) < 0)
             goto cleanup;
     }
 
-    if (!(ret = parallelsAddRoutedNetwork(privconn)))
+    if (parallelsAddRoutedNetwork(privconn) < 0)
         goto cleanup;
 
     ret = 0;

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list