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

[Daniel P. Berrange]

On Wed, Oct 07, 2009 at 10:05:40AM -0400, Laine Stump wrote:
> From: root <root at 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

> @@ -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)
>          return(0);
>      virBufferVSprintf(buf, "  <protocol family='%s'>\n", def->proto.family);
> @@ -1015,20 +1013,20 @@ virInterfaceProtocolDefFormat(virConnectPtr conn ATTRIBUTE_UNUSED,
>              virBufferAddLit(buf, "    <dhcp peerdns='yes'/>\n");
>          else
>              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");


But should prefix really be optional ? 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.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|