[libvirt] [PATCHv7 3/4] domifaddr: Implement the API for qemu

[Nehal J Wani]

On Wed, Dec 17, 2014 at 11:56 AM, Wang Rui <moon.wangrui at huawei.com> wrote:
> On 2014/12/17 8:16, Nehal J Wani wrote:
>> By querying the qemu guest agent with the QMP command
>> "guest-network-get-interfaces" and converting the received JSON
>> output to structured objects.
>>
>> Although "ifconfig" is deprecated, IP aliases created by "ifconfig"
>> are supported by this API. The legacy syntax of an IP alias is:
>> "<ifname>:<alias-name>". Since we want all aliases to be clubbed
>> under parent interface, simply stripping ":<alias-name>" suffices.
>> Note that IP aliases formed by "ip" aren't visible to "ifconfig",
>> and aliases created by "ip" do not have any specific name. But
>> we are lucky, as qemu guest agent detects aliases created by both.
>>
> [...]
>
>> +static int
>> +qemuGetDHCPInterfaces(virDomainPtr dom,
>> +                      virDomainObjPtr vm,
>> +                      virDomainInterfacePtr **ifaces)
>> +{
>> +    int rv = -1;
>> +    int n_leases = 0;
>> +    size_t i, j;
>> +    size_t ifaces_count = 0;
>> +    virNetworkPtr network;
>> +    char macaddr[VIR_MAC_STRING_BUFLEN];
>> +    virDomainInterfacePtr iface = NULL;
>> +    virNetworkDHCPLeasePtr *leases = NULL;
>> +    virDomainInterfacePtr *ifaces_ret = NULL;
>> +
>> +    for (i = 0; i < vm->def->nnets; i++) {
>> +        virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr);
>> +        network = virNetworkLookupByName(dom->conn,
>> +                                         vm->def->nets[i]->data.network.name);
>> +
>> +        if (dom->conn->networkDriver &&
>> +            dom->conn->networkDriver->networkGetDHCPLeases) {
> Is it better to move this check out of the loop?

Yup, seems like a redundant check for each loop. Will fix that.

>
>> +            n_leases = virNetworkGetDHCPLeases(network, macaddr,
>> +                                         &leases, 0);
> Memory is allocated for 'leases' in the loop 'nnets' times. But it is freed only
> once at the end. Should it be freed in the loop, not at the end of the function?
>

Whoops! Thats a huge leak! Will fix it in the next revision.

>> +            if (n_leases < 0)
>> +                goto error;
>> +        }
>> +
>> +        if (n_leases) {
>> +            if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
>> +                goto error;
>> +
>> +            if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0)
>> +                goto error;
>> +
>> +            iface = ifaces_ret[ifaces_count - 1];
>> +            /* Assuming each lease corresponds to a separate IP */
>> +            iface->naddrs = n_leases;
>> +
>> +            if (VIR_ALLOC_N(iface->addrs, iface->naddrs) < 0)
>> +                goto error;
>> +
>> +            if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0)
>> +                goto cleanup;
>> +
>> +            if (VIR_STRDUP(iface->hwaddr, macaddr) < 0)
>> +                goto cleanup;
>> +        }
>> +
>> +        for (j = 0; j < n_leases; j++) {
>> +            virNetworkDHCPLeasePtr lease = leases[j];
>> +            virDomainIPAddressPtr ip_addr = &iface->addrs[j];
>> +
>> +            if (VIR_STRDUP(ip_addr->addr, lease->ipaddr) < 0)
>> +                goto cleanup;
>> +
>> +            ip_addr->type = lease->type;
>> +            ip_addr->prefix = lease->prefix;
>> +        }
>> +    }
>> +
>> +    *ifaces = ifaces_ret;
>> +    ifaces_ret = NULL;
>> +    rv = ifaces_count;
>> +
>> + cleanup:
>> +    for (i = 0; i < n_leases; i++)
>> +        virNetworkDHCPLeaseFree(leases[i]);
>> +
>> +    VIR_FREE(leases);
>> +    return rv;
>> +
>> + error:
>> +    if (ifaces_ret) {
>> +        for (i = 0; i < ifaces_count; i++)
>> +            virDomainInterfaceFree(ifaces_ret[i]);
>> +    }
>> +    VIR_FREE(ifaces_ret);
>> +
>> +    goto cleanup;
>> +}
>
>
>



-- 
Nehal J Wani