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

Re: [libvirt] [PATCH v2 1/8] Introduce virDomainInterfacesAddresses API



On 01/03/2013 08:46 AM, Michal Privoznik wrote:
> This API returns dynamically allocated array of IP addresses for
> all domain interfaces.
> ---
>  include/libvirt/libvirt.h.in | 49 ++++++++++++++++++++++++++++++
>  python/generator.py          |  2 ++
>  src/driver.h                 |  6 ++++
>  src/libvirt.c                | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_public.syms      |  6 ++++
>  5 files changed, 134 insertions(+)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index cc3fe13..3ee6688 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -4563,6 +4563,55 @@ int virDomainFSTrim(virDomainPtr dom,
>                      unsigned long long minimum,
>                      unsigned int flags);
>  
> +typedef enum {
> +    VIR_INTERFACE_BROADCAST = (1 << 0), /* Broadcast address valid. */
> +    VIR_INTERFACE_PPP = (1 << 1), /* Interface is point-to-point. */

I think you mean VIR_INTERFACE_PTP, not _PPP.


> +    /* we can add new flags here */
> +} virDomainInterfaceType;
> +
> +typedef enum {
> +    VIR_IP_ADDR_TYPE_IPV4,
> +    VIR_IP_ADDR_TYPE_IPV6,
> +
> +#ifdef VIR_ENUM_SENTINELS
> +    VIR_IP_ADDR_TYPE_LAST
> +#endif
> +} virIPAddrType;
> +
> +
> +typedef struct _virDomainInterfaceIPAddress virDomainIPAddress;
> +typedef virDomainIPAddress *virDomainIPAddressPtr;
> +struct _virDomainInterfaceIPAddress {
> +    int type;       /* virIPAddrType */
> +    char *addr;     /* IP address */
> +    int prefix;     /* IP address prefix */
> +    char *dstaddr;  /* Broadcast address (if @flags & VIR_INTERFACE_BROADCAST),
> +                       PPP destination address (if @flags & VIR_INTERFACE_PPP) */
> +};

I dislike creating yet another struct to represent IP address info when
we already can do that internally with virSocketAddr, but virSocketAddr
is too complicated (and has platform-specific bits to boot) to put it in
the public API, and every other IP address in the public API is
represented with XML, so I guess this is okay.

> +
> +typedef struct _virDomainInterface virDomainInterface;
> +typedef virDomainInterface *virDomainInterfacePtr;
> +struct _virDomainInterface {
> +    char *name;                     /* interface name */
> +    unsigned int flags;             /* virDomainInterfaceType */
> +    char *hwaddr;                   /* hardware address */
> +    unsigned int ip_addrs_count;    /* number of items in @ip_addr */
> +    virDomainIPAddressPtr ip_addrs; /* array of IP addresses */
> +};
> +
> +typedef enum {
> +    VIR_DOMAIN_INTERFACE_ADDRS_DEFAULT      = 0, /* hypervisor choice */
> +    VIR_DOMAIN_INTERFACE_ADDRS_GUEST_AGENT  = 1, /* use guest agent */
> +    VIR_DOMAIN_INTERFACE_ADDRS_NWFILTER     = 2, /* use nwfilter learning code */
> +    /* etc ... */

You should remove the "etc" line.

> +} virDomainInterfacesAddressesMethod;
> +
> +
> +int virDomainInterfacesAddresses(virDomainPtr domain,
> +                                 virDomainInterfacePtr **ifaces,
> +                                 unsigned int method,
> +                                 unsigned int flags);
> +
>  /**
>   * virSchedParameterType:
>   *
> diff --git a/python/generator.py b/python/generator.py
> index bae4edc..ee39e51 100755
> --- a/python/generator.py
> +++ b/python/generator.py
> @@ -243,6 +243,7 @@ skipped_types = {
>       'virEventHandleCallback': "No function types in python",
>       'virEventTimeoutCallback': "No function types in python",
>       'virDomainBlockJobInfoPtr': "Not implemented yet",
> +     'virDomainInterfacePtr': "Not implemented yet",
>  }
>  
>  #######################################################################
> @@ -428,6 +429,7 @@ skip_impl = (
>      'virNodeGetMemoryParameters',
>      'virNodeSetMemoryParameters',
>      'virNodeGetCPUMap',
> +    'virDomainInterfacesAddresses',
>  )
>  
>  qemu_skip_impl = (
> diff --git a/src/driver.h b/src/driver.h
> index 64d652f..dc93ffb 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -914,6 +914,11 @@ typedef int
>                            const char *mountPoint,
>                            unsigned long long minimum,
>                            unsigned int flags);
> +typedef int
> +    (*virDrvDomainInterfacesAddresses)(virDomainPtr domain,
> +                                       virDomainInterfacePtr **ifaces,
> +                                       unsigned int method,
> +                                       unsigned int flags);
>  
>  /**
>   * _virDriver:
> @@ -1107,6 +1112,7 @@ struct _virDriver {
>      virDrvNodeGetCPUMap                 nodeGetCPUMap;
>      virDrvDomainFSTrim                  domainFSTrim;
>      virDrvDomainSendProcessSignal       domainSendProcessSignal;
> +    virDrvDomainInterfacesAddresses     domainInterfacesAddresses;
>  };
>  
>  typedef int
> diff --git a/src/libvirt.c b/src/libvirt.c
> index bf674d1..6f0de36 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -20424,3 +20424,74 @@ error:
>      virDispatchError(dom->conn);
>      return -1;
>  }
> +
> +/**
> + * virDomainInterfacesAddresses:


The repeated plurality sounds a bit clumsy, but it *is* the most correct
I guess (since virDomainInterfaceAddresses would imply that it's only
returning the addresses of a single interface). However, most
(unfortunately not all) libvirt APIs that retrieve information about an
entity include a verb in the name, usually "Get". So how about naming it
"virDomainGetInterfacesAddresses" (following all the other virDomainGet*
function names).

(BTW, it seems a bit bothersome to me that we will now have
virDomainGetInterfaceParameters() (which merely returns duplicates of
bandwidth information already present in the domain XML for a single
interface), virDomainInterfaceStats() (which returns traffic stats for a
single interface), and this new function, which returns addresses for
all interfaces. It's true that the source of each piece of information
is different (the first is from the domain's configuration, the 2nd is
from ioctls run on the host, and the third is from either querying the
guest agent, or from asking nwfilter for the value it's learned via
snooping the traffic), but that's an implementation detail that
shouldn't matter to the user of the libvirt API. It would be nice if
this was somehow consolidated, although I'm not expecting you to do it :-))

> + * @domain: domain object
> + * @ifaces: array of @domain interfaces
> + * @method: which method use, one of virDomainInterfacesAddressesMethod
> + * @flags: extra flags, not used yet, so callers should always pass 0
> + *
> + * Return an array of interfaces presented in given @domain among with
     s/presented in given/present in the given/
     s/among/along/

> + * their IP and HW addresses. Single interface can have multiple or even
> + * none IP address.

    s/Single/A single/
    s/none/no/


> + *
> + * This API dynamically allocates the virDomainInterfacePtr struct based on

s/struct/object/

> + * how much interfaces domain has, usually there's 1:1 correlation between

   s/much interfaces domain/many interfaces the domain/

> + * interfaces seen from the host and interfaces seen from the guest. The
> + * count of elements allocated is then returned.
> + *
> + * Depending on selected @method, guest agent or NWFilter can be used. The
> + * guest agent method communicates with a running agent within guest and
> + * dumps all interfaces within their addresses. This, obviously requires
> + * guest agent to be configured and running and will fail otherwise.
> + * However, this gives interfaces from guest point of view, that is, names
> + * and HW addresses are those as seen from the guest.  When the NWFilter
> + * method is used, libvirt starts snooping on guests interfaces and try to
> + * gather as much info as possible. This returns interface name and HW
> + * address as seen from the host perspective.

Depending on the selected @method, the address information will be
retrieved from a different source: the guest agent method requests the
information from an agent running in the guest. This obviously requires
a guest agent to be configured and running, and will otherwise fail.
However, the guest agent method provides the information from the
guest's point of view - the interface names and hardware addresses are
those seen by the guest OS. When the NWFilter method is used, the
information is requested from libvirt's NWFilter driver, which "snoops"
traffic on the guest's network connections and deduces IP and MAC
address information from examining that traffic; in this case, the
interface name will be the name of the tap device created by libvirt to
connect the device to the host's network, *not* the name of the device
as seen by the guest OS.


> + *
> + * @ifaces->name is never NULL, other pointers MIGHT be NULL.
> + *
> + * The caller *must* free @ifaces when no longer needed.
> + *
> + * Returns -1 on failure
> + *         otherwise the count of items stored into @ifaces.
> + */
> +int
> +virDomainInterfacesAddresses(virDomainPtr domain,
> +                             virDomainInterfacePtr **ifaces,
> +                             unsigned int method,
> +                             unsigned int flags)
> +{
> +    virConnectPtr conn;
> +
> +    VIR_DOMAIN_DEBUG(domain, "ifaces=%p, methd=%d, flags=%x",
> +                     ifaces, method, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> +        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    conn = domain->conn;
> +
> +    virCheckNonNullArgGoto(ifaces, error);
> +
> +    if (conn->driver->domainInterfacesAddresses) {
> +        int ret;
> +        ret = conn->driver->domainInterfacesAddresses(domain, ifaces,
> +                                                      method, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    virDispatchError(domain->conn);
> +    return -1;
> +}
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index e3d63d3..8e7c5d2 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -580,4 +580,10 @@ LIBVIRT_1.0.1 {
>          virDomainSendProcessSignal;
>  } LIBVIRT_1.0.0;
>  
> +LIBVIRT_1.0.2 {
> +    global:
> +        virDomainInterfacesAddresses;
> +} LIBVIRT_1.0.1;
> +
> +
>  # .... define new API here using predicted next version number ....


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