8:43 a.m.
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(a)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)
> 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");
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.