[libvirt] [PATCH] libxl: add .domainInterfaceAddresses

[Jim Fehlig]

Chun Yan Liu wrote:
> 
>>>> On 5/14/2016 at 07:47 AM, in message <5736677D.8030209 at suse.com>, Jim Fehlig
> <jfehlig at suse.com> wrote: 
>> On 05/13/2016 12:21 AM, Chunyan Liu wrote: 
>>> Add .domainInterfaceAddresses so that user can have a way to 
>>> get domain interface address by 'virsh domifaddr'. Currently 
>>> it only supports '--source lease'. 
>>>
>>> Signed-off: Chunyan Liu <cyliu at suse.com> 
>>> --- 
>>>  src/libxl/libxl_driver.c | 140  
>> +++++++++++++++++++++++++++++++++++++++++++++++ 
>>>  1 file changed, 140 insertions(+) 
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c 
>>> index 062d6f8..f2bd6fa 100644 
>>> --- a/src/libxl/libxl_driver.c 
>>> +++ b/src/libxl/libxl_driver.c 
>>> @@ -5425,6 +5425,145 @@ static int libxlNodeGetSecurityModel(virConnectPtr  
>> conn, 
>>>      return 0; 
>>>  } 
>>>   
>>> +static int 
>>> +libxlGetDHCPInterfaces(virDomainPtr dom, 
>>> +                       virDomainObjPtr vm, 
>>> +                       virDomainInterfacePtr **ifaces) 
>>> +{ 
>>> +    int rv = -1; 
>>> +    int n_leases = 0; 
>>> +    size_t i, j; 
>>> +    size_t ifaces_count = 0; 
>>> +    virNetworkPtr network = NULL; 
>>> +    char macaddr[VIR_MAC_STRING_BUFLEN]; 
>>> +    virDomainInterfacePtr iface = NULL; 
>>> +    virNetworkDHCPLeasePtr *leases = NULL; 
>>> +    virDomainInterfacePtr *ifaces_ret = NULL; 
>>> + 
>>> +    if (!dom->conn->networkDriver || 
>>> +        !dom->conn->networkDriver->networkGetDHCPLeases) { 
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", 
>>> +                       _("Network driver does not support DHCP lease  
>> query")); 
>>> +        return -1; 
>>> +    } 
>>> + 
>>> +    for (i = 0; i < vm->def->nnets; i++) { 
>>> +        if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) 
>>> +            continue; 
>>> + 
>>> +        virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); 
>>> +        virObjectUnref(network); 
>>> +        network = virNetworkLookupByName(dom->conn, 
>>> +                                         vm->def->nets[i]->data.network.name); 
>>> + 
>>> +        if ((n_leases = virNetworkGetDHCPLeases(network, macaddr, 
>>> +                                                &leases, 0)) < 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; 
>>> +        } 
>>> + 
>>> +        for (j = 0; j < n_leases; j++) 
>>> +            virNetworkDHCPLeaseFree(leases[j]); 
>>> + 
>>> +        VIR_FREE(leases); 
>>> +    } 
>>> + 
>>> +    *ifaces = ifaces_ret; 
>>> +    ifaces_ret = NULL; 
>>> +    rv = ifaces_count; 
>>> + 
>>> + cleanup: 
>>> +    virObjectUnref(network); 
>>> +    if (leases) { 
>>> +        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; 
>>> +} 
>>  
>> It's unfortunate this is a copy-paste from the qemu driver. The code is not 
>> trivial and any bug fixes in one copy could be missed in the other. A lot of  
>> the 
>> function is domain related, so probably can't be abstracted further to the 
>> network code. Have you considered approaches that allow the drivers to share 
>> this code?
> 
> Well, it can be extracted and placed in bridge_driver.c as networkGetDHCPInterfaces,
> but I don't know if that is acceptable?

Hmm, maybe something like networkGetDHCPLeasesAll() is a better name. Regardless
of the name, I see that other functions in bridge_driver.c take a
virDomainDefPtr, so maybe extracting the code to bridge_driver.c is acceptable.

Laine (cc'd) is generally regarded as the libvirt networking expert :-). Let's
see if he has an opinion.

Regards,
Jim