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

[Laine Stump]

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 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
>    


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.