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

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




>>> 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?

-Chunyan


>  
> (NB: for any "super double secret API" network driver function you  
> implement, you need to implement and alternative NOP function that is  
> used when the bridge driver is disabled. See bridge_driver.h:68 for  
> examples.) 
>  
>  
>  



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