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

Re: [libvirt] [PATCH 06/10] linkstate: Add parsing code for new XML element



On 08/11/2011 09:27 AM, Peter Krempa wrote:
---
  src/conf/domain_conf.c |   19 +++++++++++++++++++
  src/conf/domain_conf.h |    1 +
  2 files changed, 20 insertions(+), 0 deletions(-)

This patch is useful, even without the new API.

@@ -3077,6 +3081,14 @@ virDomainNetDefParseXML(virCapsPtr caps,
          }
      }

+    def->linkstate = VIR_LINK_STATE_DEFAULT;
+    if (linkstate != NULL) {
+        if (STREQ(linkstate, "down"))
+            def->linkstate = VIR_LINK_STATE_DOWN;
+        else
+            def->linkstate = VIR_LINK_STATE_UP;

Do we really want to convert all other strings to STATE_UP, or should we specifically check for "up" and reject unknown strings?

@@ -9019,6 +9032,12 @@ virDomainNetDefFormat(virBufferPtr buf,
          virBufferAddLit(buf,   "</tune>\n");
      }

+
+    if (def->linkstate == VIR_LINK_STATE_DOWN)
+        virBufferAddLit(buf,   "<link state='down'/>\n");
+    if (def->linkstate == VIR_LINK_STATE_UP)

This could be 'else if', but not a big deal to micro-optimize.

+        virBufferAddLit(buf,   "<link state='up'/>\n");
+
      if (virBandwidthDefFormat(buf, def->bandwidth, "      ")<  0)
          return -1;

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index abf9cbd..4655563 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -427,6 +427,7 @@ struct _virDomainNetDef {
      char *filter;
      virNWFilterHashTablePtr filterparams;
      virBandwidthPtr bandwidth;
+    unsigned int linkstate;

Why unsigned? As currently used, you could get away with a bool. Would it be better to introduce a VIR_ENUM for link states, especially if we ever think it might be worth adding other link states? (I know that some 10/100/1000 NIC hardware has a state representing autonegotiation, where a remote end has been detected but where the link has not yet settled on which speed to use, although I have no idea if that hardware state is ever exposed to the userspace by the kernel, let alone something that can be emulated). If you do use VIR_ENUM, then our convention has been to use 'int', not 'unsigned int'.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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