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

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



On 05/25/2016 08:52 AM, Laine Stump wrote:
> On 05/24/2016 11:42 PM, Jim Fehlig wrote:
>> 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.
>
> Yeah, I sadly agree with you :-(. There are just too many things connected to
> it to make it easy to put in a category, and there's enough violations of the
> boundaries/referencing directions between conf, util, network driver, and
> hypervisor drivers already; I'd feel better about reducing them rather than
> increasing them.

Thanks for the confirmation. The patch is fine otherwise, so I've pushed it now.

Regards,
Jim


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