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

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



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


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