[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv5 3/5] domifaddr: Implement the API for qemu



On Sun, Sep 01, 2013 at 07:13:33PM +0530, Nehal J Wani wrote:
> +int
> +qemuAgentGetInterfaces(qemuAgentPtr mon,
> +                       virDomainInterfacePtr **ifaces)
> +{
> +
> +        /* interface name is required to be presented */
> +        name = virJSONValueObjectGetString(tmp_iface, "name");
> +        if (!name) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("qemu agent didn't provide 'name' field"));
> +            goto error;
> +        }
> +
> +        /* Handle aliases of type <ifname>:<alias-name> */
> +        ifname = virStringSplit(name, ":", 2);
> +        ifname_s = ifname[0];
> +
> +        iface = virHashLookup(ifaces_store, ifname_s);
> +
> +        /* If the storage bag doesn't contain this iface, add it */
> +        if (!iface) {
> +            if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
> +                goto cleanup;
> +
> +            if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0)
> +                goto cleanup;
> +
> +            if (virHashAddEntry(ifaces_store, ifname_s,
> +                                ifaces_ret[ifaces_count - 1]) < 0)
> +                goto cleanup;
> +
> +            iface = ifaces_ret[ifaces_count - 1];
> +            iface->naddrs = 0;
> +
> +            if (VIR_STRDUP(iface->name, ifname_s) < 0)
> +                goto error;
> +
> +            /* hwaddr might be omitted */

Really ? Can qemu guest agent report interfacs with 'hardware-address'
field not being present ?  I'd expect that field to be mandatory and
so report an error in libvirt if missing.

> +            hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address");
> +            if (hwaddr && VIR_STRDUP(iface->hwaddr, hwaddr) < 0)
> +                goto error;
> +        }
> +
> +        /* Has to be freed for each interface. */
> +        virStringFreeList(ifname);

You're leaking 'ifname' if any of the 'goto xxxx' branches are
taken above.


> +
> +        /* as well as IP address which - moreover -
> +         * can be presented multiple times */
> +        ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses");
> +        if (!ip_addr_arr)
> +            continue;
> +
> +        if ((ip_addr_arr_size = virJSONValueArraySize(ip_addr_arr)) < 0)
> +            /* Mmm, empty 'ip-address'? */
> +            continue;

The '< 0' condition indicates an error scenario, so shouldn't be
silently ignored.  '== 0' is the empty list scenario that is ok
to ignore, but already handled by the for() loop conditions.

> +
> +        /* If current iface already exists, continue with the count */
> +        addrs_count = iface->naddrs;
> +
> +        for (j = 0; j < ip_addr_arr_size; j++) {
> +



> diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
> index 4e27981..4014a09 100644
> --- a/tests/qemuagenttest.c
> +++ b/tests/qemuagenttest.c
> @@ -576,6 +576,154 @@ cleanup:
>      return ret;
>  }
>  
> +static const char testQemuAgentGetInterfacesResponse[] =
> +    "{\"return\": "
> +    "    ["
> +    "       {\"name\":\"lo\","
> +    "        \"ip-addresses\":"
> +    "          ["
> +    "             {\"ip-address-type\":\"ipv4\","
> +    "              \"ip-address\":\"127.0.0.1\","
> +    "              \"prefix\":8"
> +    "             },"
> +    "             {\"ip-address-type\":\"ipv6\","
> +    "              \"ip-address\":\"::1\","
> +    "              \"prefix\":128"
> +    "             }"
> +    "          ],"
> +    "        \"hardware-address\":\"00:00:00:00:00:00\""
> +    "       },"
> +    "       {\"name\":\"eth0\","
> +    "        \"ip-addresses\":"
> +    "          ["
> +    "             {\"ip-address-type\":\"ipv4\","
> +    "              \"ip-address\":\"192.168.102.142\","
> +    "              \"prefix\":24"
> +    "             },"
> +    "             {\"ip-address-type\":\"ipv6\","
> +    "              \"ip-address\":\"fe80::5054:ff:fe89:ad35\","
> +    "              \"prefix\":64"
> +    "             },"
> +    "             {\"ip-address-type\":\"ipv4\","
> +    "              \"ip-address\":\"192.168.234.152\","
> +    "              \"prefix\":16"
> +    "             },"
> +    "             {\"ip-address-type\":\"ipv6\","
> +    "              \"ip-address\":\"fe80::5054:ff:fec3:68bb\","
> +    "              \"prefix\":64"
> +    "             }"
> +    "          ],"
> +    "        \"hardware-address\":\"52:54:00:89:ad:35\""
> +    "       },"
> +    "       {\"name\":\"eth1\","
> +    "        \"ip-addresses\":"
> +    "          ["
> +    "             {\"ip-address-type\":\"ipv4\","
> +    "              \"ip-address\":\"192.168.103.83\","
> +    "              \"prefix\":24"
> +    "             },"
> +    "             {\"ip-address-type\":\"ipv6\","
> +    "              \"ip-address\":\"fe80::5054:ff:fed3:39ee\","
> +    "              \"prefix\":64"
> +    "             }"
> +    "          ],"
> +    "        \"hardware-address\":\"52:54:00:d3:39:ee\""
> +    "       },"
> +    "       {\"name\":\"eth1:0\","
> +    "        \"ip-addresses\":"
> +    "          ["
> +    "             {\"ip-address-type\":\"ipv4\","
> +    "              \"ip-address\":\"192.168.10.91\","
> +    "              \"prefix\":24"
> +    "             },"
> +    "             {\"ip-address-type\":\"ipv6\","
> +    "              \"ip-address\":\"fe80::fc54:ff:fefe:4c4f\","
> +    "              \"prefix\":64"
> +    "             }"
> +    "          ],"
> +    "        \"hardware-address\":\"52:54:00:d3:39:ee\""
> +    "       },"
> +    "       {\"name\":\"eth2\","
> +    "        \"hardware-address\":\"52:54:00:36:2a:e5\""
> +    "       }"
> +    "    ]"
> +    "}";

I'd re-arrange these a little, eg so that 'eth2' comes
before 'eth1:1', just so that we validate that we're
coping with things in a non-obvious sort order.

> +
> +static int
> +testQemuAgentGetInterfaces(const void *data)
> +{
> +    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
> +    qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
> +    size_t i;
> +    int ret = -1;
> +    int ifaces_count = 0;
> +    virDomainInterfacePtr *ifaces = NULL;
> +
> +    if (!test)
> +        return -1;
> +
> +    if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
> +        goto cleanup;
> +
> +    if (qemuMonitorTestAddItem(test, "guest-network-get-interfaces",
> +                               testQemuAgentGetInterfacesResponse) < 0)
> +        goto cleanup;
> +
> +    if ((ifaces_count = qemuAgentGetInterfaces(qemuMonitorTestGetAgent(test),
> +                                               &ifaces)) < 0)
> +        goto cleanup;
> +
> +    if (ifaces_count != 4) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "expected 4 interfaces, got %d", ret);
> +        goto cleanup;
> +    }
> +
> +    if (ifaces[0]->naddrs != 2 ||
> +        ifaces[1]->naddrs != 4 ||
> +        ifaces[2]->naddrs != 4 ||
> +        ifaces[3]->naddrs != 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       "unexpected return value for number of IP addresses");
> +        goto cleanup;
> +    }
> +
> +    if (STRNEQ(ifaces[0]->name, "lo") ||
> +        STRNEQ(ifaces[0]->addrs[0].addr, "127.0.0.1") ||
> +        ifaces[0]->addrs[1].prefix != 128) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       "unexpected return values for interface: lo");
> +        goto cleanup;
> +    }
> +
> +    if (STRNEQ(ifaces[1]->hwaddr, "52:54:00:89:ad:35") ||
> +        ifaces[1]->addrs[0].prefix != 24 ||
> +        ifaces[1]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV6) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       "unexpected return values for interface: eth0");
> +        goto cleanup;
> +    }
> +
> +    if (STRNEQ(ifaces[2]->name, "eth1") ||
> +        ifaces[2]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV4 ||
> +        STRNEQ(ifaces[2]->addrs[1].addr, "fe80::5054:ff:fed3:39ee")) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       "unexpected return values for interface: eth1");
> +        goto cleanup;
> +    }

You're only validating a couple of the IP addresses here & MAC addrs
and names. I'd like to see all of them validated for all NIC. This
would give us higher confidence that we're correctly merging eth1
and eth1:0 together.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]