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

Re: [libvirt] [PATCH 1/2] Support reporting live interface IP/netmask.

On 10/08/2009 06:25 AM, Daniel P. Berrange wrote:
On Wed, Oct 07, 2009 at 10:05:40AM -0400, Laine Stump wrote:
From: root<root vlap laine org>

This patch adds the flag VIR_INTERFACE_XML_INACTIVE to
virInterfaceGetXMLDesc's flags. When it is*not* set (the default), the
live interface info will be returned in the XML (in particular, the IP
address(es) and netmask(s) will be retrieved by querying the interface
directly, rather than  reporting what's in the config file). The
backend of this is in netcf's ncf_if_xml_state() function.

Note that you need at least netcf 0.1.2 for this to work correctly.
The configure.ac pkgconfig check for netcf should be updated to
match the new minimal requirement here, otherwise people will get
a compile error, rather than a clear message about required version.
Similarly for the BuildRequires in libvirt.spec.in

And I'm not even sure why I didn't go ahead and do that :-P. I guess the netcf release wasn't done yet when I first packaged up the patches, and I didn't think to go back and do it once the release was made.

@@ -1005,6 +1002,7 @@ virInterfaceVlanDefFormat(virConnectPtr conn, virBufferPtr buf,
  static int
  virInterfaceProtocolDefFormat(virConnectPtr conn ATTRIBUTE_UNUSED,
                                virBufferPtr buf, const virInterfaceDefPtr def) {
+    char prefixStr[16];
      if (def->proto.family == NULL)
      virBufferVSprintf(buf, "<protocol family='%s'>\n", def->proto.family);
@@ -1015,20 +1013,20 @@ virInterfaceProtocolDefFormat(virConnectPtr conn ATTRIBUTE_UNUSED,
              virBufferAddLit(buf, "<dhcp peerdns='yes'/>\n");
              virBufferAddLit(buf, "<dhcp/>\n");
-    } else {
-        /* theorically if we don't have dhcp we should have an address */
-        if (def->proto.address != NULL) {
-            if (def->proto.prefix != 0)
-                virBufferVSprintf(buf, "<ip address='%s' prefix='%d'/>\n",
-                                  def->proto.address, def->proto.prefix);
-            else
-                virBufferVSprintf(buf, "<ip address='%s'/>\n",
-                                  def->proto.address);
-        }
-        if (def->proto.gateway != NULL) {
-            virBufferVSprintf(buf, "<route gateway='%s'/>\n",
-                              def->proto.gateway);
-        }
+    }
+    if (def->proto.address != NULL) {
+        if (def->proto.prefix != 0)
+            snprintf(prefixStr, sizeof(prefixStr), "%d", def->proto.prefix);
+        virBufferVSprintf(buf, "<ip address='%s'%s%s%s%s%s%s/>\n",
+                          def->proto.address,
+                          def->proto.prefix ? " prefix='"       : "",
+                          def->proto.prefix ? prefixStr         : "",
+                          def->proto.prefix ? "'"               : "");
+    }
This is a rather unreadable way to format the XML, and has a rather
redundant extra snprintf call there. I think it'd look better as

      virBufferVSprintf(buf, "<ip address='%s'", def->proto.address);
      if (def->proto.prefix)
          virBufferVSprintf(buf, " prefix='%d'", def->proto.prefix);
      virBufferVSprintf(buf, "/>\n");

Yeah, I had made the same decision and changed it to pretty much what you suggest in [PATCH 2/2], but didn't redo this first patch because it was getting immediately overridden anyway and I didn't want to deal with the merge conflict. I'll redo it in the next iteration though, so there won't be any snapshot of code available that's unsightly ;-)

But should prefix really be optional ?

Sure. RFC something-or-other-from-eons-ago-before-CIDR spells out the netmask to use when one isn't specified for any given IP address (class A, B, C). Certainly you can give ifconfig an IP with no netmask/prefix and it will do the right thing.

You really need a prefix to
meaningfully interpret an address. If it is left out of the original
sysconfig file in /etc/sysconfig/network-scripts, there are defined
default values that should be used. IMHO, libvirt output XML should
always include the prefix, otherwise applications all have to add
the logic to calculate default prefix values themselves.  So I'd
prefer to see prefix mandatory in the XML we output, optional in
the XML we accept for input.

Okay, that sounds reasonable.

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