[libvirt] [PATCH v2 1/5] Introduce virDomainGetInterfacesAddresses API

Peter Krempa pkrempa at redhat.com
Mon Jun 25 16:06:20 UTC 2012


On 06/21/12 15:55, Michal Privoznik wrote:
> This API returns dynamically allocated XML document
> showing IP addresses for all domain interfaces.
> ---
>   docs/schemas/interfaces.rng  |   57 ++++++++++++++++++++++++++++++++++++++++++
>   include/libvirt/libvirt.h.in |    2 +
>   src/driver.h                 |    4 +++
>   src/libvirt.c                |   49 ++++++++++++++++++++++++++++++++++++
>   src/libvirt_public.syms      |    1 +
>   src/remote/remote_driver.c   |    1 +
>   src/remote/remote_protocol.x |   12 ++++++++-
>   src/remote_protocol-structs  |    8 ++++++
>   8 files changed, 133 insertions(+), 1 deletions(-)
>   create mode 100644 docs/schemas/interfaces.rng
>
> diff --git a/docs/schemas/interfaces.rng b/docs/schemas/interfaces.rng
> new file mode 100644
> index 0000000..95c939e
> --- /dev/null
> +++ b/docs/schemas/interfaces.rng
> @@ -0,0 +1,57 @@

Missing XML doc header:

<?xml version="1.0"?>


> +<!-- A Relax NG schema for the libvirt interfaces (return of
> +     virDomainGetInterfacesAddresses) XML format -->
> +<grammar xmlns="http://relaxng.org/ns/structure/1.0"
> +    datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes">
> +  <include href='basictypes.rng'/>
> +  <start>
> +    <ref name='interfaces'/>
> +  </start>
> +
> +
> +  <define name='interfaces'>
> +    <element name='interfaces'>
> +      <zeroOrMore>
> +        <ref name='interface'/>
> +      </zeroOrMore>
> +    </element>
> +  </define>
> +
> +  <define name='interface'>
> +    <element name='interface'>
> +      <element name='name'>
> +        <text/>
> +      </element>
> +      <optional>
> +        <element name='hwaddr'>
> +          <text/>
> +        </element>
> +      </optional>
> +      <optional>
> +        <ref name='addresses'/>
> +      </optional>
> +    </element>
> +  </define>
> +
> +  <define name='addresses'>
> +    <element name='addresses'>
> +      <zeroOrMore>
> +        <ref name='ip'/>
> +      </zeroOrMore>
> +    </element>
> +  </define>
> +
> +  <define name='ip'>
> +    <element name='ip'>
> +      <attribute name='type'>
> +        <choice>
> +          <value>ipv4</value>
> +          <value>ipv6</value>
> +        </choice>
> +      </attribute>
> +      <attribute name='prefix'>
> +        <ref name='unsignedInt'/>
> +      </attribute>
> +      <text/>
> +    </element>
> +  </define>
> +</grammar>

The XML looks reasonable to me, but I'd appreciate some other opinions 
on this as this is a architectural decision that we'll have to support 
forever. (Or maybe just until 21.12.2012? :))

> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 024c4ec..90660d1 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -1659,6 +1659,8 @@ int                     virDomainGetInterfaceParameters (virDomainPtr dom,
>                                                           const char *device,
>                                                           virTypedParameterPtr params,
>                                                           int *nparams, unsigned int flags);
> +char *                  virDomainGetInterfacesAddresses (virDomainPtr dom,
> +                                                        unsigned int flags);

Bad indent: missing one space. Looking at the file, I'm taking this 
objection back as the problem is worthy for separate cleanup.

>
>   /* Management of domain block devices */
>
> diff --git a/src/driver.h b/src/driver.h
> index b3c1740..b2ed4b0 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -398,6 +398,9 @@ typedef int
>                                             const char *device,
>                                             virTypedParameterPtr params,
>                                             int *nparams, unsigned int flags);
> +typedef char *
> +    (*virDrvDomainGetInterfacesAddresses) (virDomainPtr dom,
> +                                          unsigned int flags);

Bad indent: missing one space. Looking at the file, there are more of 
such problems. My cleanup of driver.h wasn't thorough enough.

>
>   typedef int
>       (*virDrvDomainMemoryStats)
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 0aa50cb..2dadd6f 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -7357,6 +7357,55 @@ error:
>   }
>
>   /**
> + * virDomainGetInterfacesAddresses:
> + * @domain: domain object
> + * @flags: extra flags; not used yet, so callers should always pass 0
> + *
> + * Return an XML document representing domain interfaces among with their IP
> + * addresses assigned. It's worth noticing that one interface can have multiple

I'd rephrase the beginning of the sentence to "Note that an interface 
can have multiple addresses or even none."

> + * addresses or even none.
> + *
> + * For some hypervisors (e.g. qemu) a configured guest agent is needed. If
> + * guest agent is used, then the interface name will be the as seen by guest
> + * OS. To match such interface with the one from @domain XML us HW address or
> + * IP range.

I'd rephrase this paragraph:
Some hypervisors (e.g. qemu) require a configured guest agent instance 
running in the guest. The guest agent may return interface names as seen 
by the guest operating system. To match them to host interfaces use the 
HW addresses and IP ranges from domain's XML.

> + *
> + * Returns an XML document or NULL on error.
> + * Callers *must* free returned pointer.
> + */
> +

> +/**
>    * virDomainMemoryStats:
>    * @dom: pointer to the domain object
>    * @stats: nr_stats-sized array of stat structures (returned)
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 2913a81..20dd08d 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -542,6 +542,7 @@ LIBVIRT_0.9.13 {
>           virDomainSnapshotIsCurrent;
>           virDomainSnapshotListAllChildren;
>           virDomainSnapshotRef;
> +        virDomainGetInterfacesAddresses;

Hopefully it'll be sorted out until the release, otherwise you'll have 
to bump these numbers.

>   } LIBVIRT_0.9.11;
>
>   # .... define new API here using predicted next version number ....

I ACK (with the XML header added) the technical part of this patch but 
I'd prefer if someone could do a architectural and language review.

Peter




More information about the libvir-list mailing list