[libvirt] [PATCH V14 5/5] nwfilter: Display detected IP address in domain XML

[Eric Blake]

On 05/25/2012 05:56 AM, Stefan Berger wrote:
> Display detected IP addresses in the domain XML using the
> IP_LEASE variable name. This variable name now becomes
> a reserved variable name that can be read only but not set
> by the user.
> 
> The format of the value is: <ip addresss>,<lease timeout in seconds>

s/addresss/address/

> 
> An example of a displayed XML may then be:
> 
>     <interface type='bridge'>
>       <mac address='52:54:00:68:e3:90'/>
>       <source bridge='virbr0'/>
>       <target dev='vnet1'/>
>       <model type='virtio'/>
>       <filterref filter='clean-traffic'>
>         <parameter name='CTRL_IP_LEARNING' value='dhcp'/>
>         <parameter name='IP_LEASE' value='192.168.122.210,100'/>

It's a shame that we are making the user parse the single XML entity to
get two pieces of information.  I wonder if it would be better to have:

<parameter name='IP_LEASE_ADDR' value='192.168.122.210'/>
<parameter name='IP_LEASE_TIMEOUT' value='100'/>

and reserve two variable names rather than one.

I'll review the code as is, but I would get a second opinion from one of
the Dan's on whether we should change the XML to make xpath queries
easier (basically, XML that crams 2 pieces of information into one
attributes is always harder to use).

>  # define NWFILTER_VARNAME_IP "IP"
>  # define NWFILTER_VARNAME_MAC "MAC"
>  # define NWFILTER_VARNAME_CTRL_IP_LEARNING "CTRL_IP_LEARNING"
>  # define NWFILTER_VARNAME_DHCPSERVER "DHCPSERVER"
> +# define NWFILTER_VARNAME_IP_LEASE "IP_LEASE"
> +# define NWFILTER_VARNAME_IPV6_LEASE "IPV6_LEASE" /* future */

This list may need to change based on the above outcome.


> @@ -764,7 +771,6 @@ err_exit:
>      return -1;
>  }
>  
> -
>  static bool

Whitespace churn.

> +++ libvirt-acl/docs/formatnwfilter.html.in
> @@ -446,6 +446,22 @@
>      </interface>
>  </pre>
>  
> +    <p>
> +       Once an IP address has been detected, the domain's interface XML
> +       will display the detected IP address and its lease expiration time
> +       in seconds. Note that the <code>IP_LEASE</code> variable is read-only
> +       and cannot be set by the user.
> +    </p>
> +<pre>
> +    <interface type='bridge'>
> +      <source bridge='virbr0'/>
> +      <filterref filter='clean-traffic'>
> +        <parameter name='CTRL_IP_LEARNING' value='dhcp'/>
> +        <parameter name='IP_LEASE' value='192.168.122.100,200'/>
> +      </filterref>
> +    </interface>

Also, if you split into two parameters (addr vs. lease expiration), then
you can omit the expiration instead of doing an output of -1 for the
cases where an expiration is not known.

> +</pre>
> +
>      <h3><a name="nwfelemsReservedVars">Reserved Variables</a></h3>
>      <p>
>        The following table lists reserved variables in use by libvirt.
> @@ -481,6 +497,17 @@
>           <td> CTRL_IP_LEARNING </td>
>           <td> The choice of the IP address detection mode </td>
>         </tr>
> +       <tr>
> +         <td> IP_LEASE </td>
> +         <td> Read-only variable displaying the detected IP lease in the
> +              format IP address,lease expiration time in seconds </td>

Missing a "since 0.9.13" designation.

Weak ack to the code, modulo the missing 'since' tag; but like I said
earlier, I'm leaning towards a v15 that splits things into two variables.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120601/eb90430f/attachment-0001.sig>