Re: [libvirt] [PATCH] Use virMacAddrCompare() in xs_internal.c

[Jim Meyering <jim@xxxxxxxxxxxx>]

john.levon@xxxxxxx wrote:
> Use virMacAddrCompare() in xs_internal.c
>
> This solves a failure to look up a net device.

Please take the time to describe the failure scenario
in enough detail that someone (maybe me) can reproduce it.
IMHO, a bug-fix patch like this is best accompanied by
a test case that fails before the patch and succeeds with it.

The patch looks ok, but doesn't need the new label.
Nor does it need to free "val" outside the loop.
I've included a suggested incremental patch below.

> diff --git a/src/xs_internal.c b/src/xs_internal.c
> --- a/src/xs_internal.c
> +++ b/src/xs_internal.c
> @@ -915,7 +915,7 @@ char *
>  char *
>  xenStoreDomainGetNetworkID(virConnectPtr conn, int id, const char *mac) {
>      char dir[80], path[128], **list = NULL, *val = NULL;

Also, though I know it's not your code, both of the above
initializations are useless and should be removed.
That would be obvious if each of the variables
was declared at first use.

> -    unsigned int maclen, len, i, num;
> +    unsigned int len, i, num;
>      char *ret = NULL;
>      xenUnifiedPrivatePtr priv;
>
> @@ -926,9 +926,6 @@ xenStoreDomainGetNetworkID(virConnectPtr
>      if (priv->xshandle == NULL)
>          return (NULL);
>      if (mac == NULL)
> -        return (NULL);
> -    maclen = strlen(mac);
> -    if (maclen <= 0)
>          return (NULL);
>
>      snprintf(dir, sizeof(dir), "/local/domain/0/backend/vif/%d", id);
> @@ -937,18 +934,20 @@ xenStoreDomainGetNetworkID(virConnectPtr
>          return(NULL);
>      for (i = 0; i < num; i++) {
>          snprintf(path, sizeof(path), "%s/%s/%s", dir, list[i], "mac");
> -        val = xs_read(priv->xshandle, 0, path, &len);
> -        if (val == NULL)
> +        if ((val = xs_read(priv->xshandle, 0, path, &len)) == NULL)
>              break;
> -        if ((maclen != len) || memcmp(val, mac, len)) {
> -            free(val);
> -        } else {
> +
> +        if (virMacAddrCompare(val, mac) == 0) {
>              ret = strdup(list[i]);
> -            free(val);
> -            break;
> +            goto out;
>          }
> -    }
> -    free(list);
> +
> +        VIR_FREE(val);
> +    }
> +
> +out:
> +    VIR_FREE(val);
> +    VIR_FREE(list);
>      return(ret);
>  }


diff --git a/src/xs_internal.c b/src/xs_internal.c
index ce707e9..1b4284f 100644
--- a/src/xs_internal.c
+++ b/src/xs_internal.c
@@ -937,16 +937,14 @@ xenStoreDomainGetNetworkID(virConnectPtr conn, int id, const char *mac) {
         if ((val = xs_read(priv->xshandle, 0, path, &len)) == NULL)
             break;

-        if (virMacAddrCompare(val, mac) == 0) {
+        bool match = (virMacAddrCompare(val, mac) == 0);
+        VIR_FREE(val);
+        if (match) {
             ret = strdup(list[i]);
-            goto out;
+            break;
         }
-
-        VIR_FREE(val);
     }

-out:
-    VIR_FREE(val);
     VIR_FREE(list);
     return(ret);
 }

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