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

Re: [libvirt] [PATCH] libxl: add .domainInterfaceAddresses



On 05/22/2016 09:34 PM, Chun Yan Liu wrote:
>
>>>> On 5/17/2016 at 11:46 PM, in message
> <2fa0cb6f-ea83-d6f3-18f8-51a671574205 laine org>, Laine Stump <laine laine org>
> wrote: 
>> On 05/16/2016 06:05 PM, Jim Fehlig wrote: 
>>> Chun Yan Liu wrote: 
>>>>>>> On 5/14/2016 at 07:47 AM, in message <5736677D 8030209 suse com>, Jim Fehlig 
>>>> <jfehlig 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 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. 
>>  
>> Well, I don't really *like* the way that those network*() functions are  
>> implemented (just by exporting a symbol, implying that any hypervisor  
>> driver that uses them must directly link the bridge driver in, rather  
>> than being able to load an alternative), but that was the most expedient  
>> way of handling the need at the time, and nobody complained about it,  
>> so... :-) 
>>  
>> Definitely duplication of code is bad (I say that although I do it a lot  
>> myself!). And if a function is all about "doing something with a network  
>> device / connection" and it will need access to the network object, I  
>> think the network driver is the place to have it. *AND* if it's  
>> something that's not needed directly in the public API, then making it  
>> available as a public API call is a bad idea (since you're then stuck  
>> with it forever). 
>>  
>> However, I don't know that I like the idea of a function in the network  
>> driver that is trawling through the virDomainDef object. It may seem  
>> like a fine distinction - returning the lease info for a single  
>> interface vs. returning the lease info for all interfaces in the domain,  
>> but it does take the co-mingling between the network and hypervisor  
>> drivers to a new level. Yes, it's true that there are already functions  
>> that are part of this "backend super double secret network API" (watch  
>> Animal House and you'll understand the reference) that take a  
>> virDomainDefPtr as an argument; but they only use it to format the  
>> domain XML and send it to the network hook script. Technically there's  
>> nothing preventing a function in the network driver from accessing every  
>> attribute of the domain, or even modifying it :-O, that doesn't mean we  
>> should do it though. 
>>  
>> I'm trying to completely recall a vague memory of something similar to  
>> this that happened in the past - something that was needed in multiple  
>> hypervisors (which would imply that it should live either in util or  
>> conf), but that also needed to call a network function (or maybe some  
>> other driver, I forget). When trying to maintain some sort of separation  
>> and rules of engagement between the various components, there tend to be  
>> cases that just don't fit within the mold. 
>>  
>> In this case, I'm wondering if maybe the duplication can be reduced by  
>> creating a function in conf (either domain_conf.c or one of its  
>> subsidiaries) that takes a *function as an argument and calls that  
>> function for each interface. Something like this: 
>>  
>> int 
>> virDomainGetDHCPInterfaces(virDomainDefPtr def,  
>> virDomainDefGetDHCPLeasesCB getLeases, 
>> virDomainInterfacePtr **ifaces) 
>> { 
>>  
>>       for (i = 0; i < def->nnets; i++) { 
>>           virDomainNetDefPtr net = def->nets[i]; 
>>  
>>           if (virDomainNetDefGetActualType(net) !=  
>> VIR_DOMAIN_NET_TYPE_NETWORK) 
>>               continue; 
>>           if ((n_leases = getLeases(net->data.network.name, net->mac,  
>> &leases)) < 0) { 
>>                OH NOES!!!!! 
>>                goto error; 
>>           if (n_leases) { 
>>                 bobloblawlawblog.com .... 
>>           } 
>>  
>>         etc etc. 
>>       } 
>>  
>> various cleanup stuff etc. 
>>  
>> } 
>>  
>> The function getLeases would be a thin (if any) wrapper around a  
>> function in the network driver called networkGetDHCPLeases(). The  
>> toplevel function in qemu and libxl would then be simple a bit of glue  
>> followed by a call to virDomainGetDHCPInterfaces() with a pointer to the  
>> appropriate getLeases function. 
>>  
>> This way we would eliminate almost all of the duplicate code (most would  
>> go into domain_conf.c, and a bit into bridge_driver.c) without needing  
>> to teach the network driver about the internal workings of a domain def. 
>>  
>> Does that make any sense? 
> Had a look at this and tried, seems hard to put into domain_conf.c:
> except for the vm->def->nets, almost all the other things are called
> from src/libvirt-domain.c or src/libvirt-network.c, not only the
> getLeases cb of a specific network, but even the virDomainInterfacePtr
> itself is defined in libvirt-domain.h and also its Free function (in case of
> error, virNetworkDHCPLeaseFree and virDomainInterfaceFree are also
> needed). Ideas?

Hrm, maybe just go with the small amount of copied code? :-)

When originally looking at the patch, I thought it might be quite disruptive to
factor it out into something that could be used by both drivers. Unless others
have a clever idea, I'm leaning towards pushing this patch as is.

Regards,
Jim


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