On 08/21/2012 11:47 PM, Daniel Veillard wrote:
> On Tue, Aug 21, 2012 at 11:26:08PM -0400, Laine Stump wrote:
> [...]
>> ===========
>> OPTION 2) have a separate API for each different type of element we may want to
>> change, e.g.:
>>
>>
>> virNetworkUpdateForwardInterface(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>> virNetworkUpdatePortgroup(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>> virNetworkUpdateIpDhcpHost(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>> virNetworkUpdateDnsEntry(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>>
>> /* The name of this one may confuse... */
>> virNetworkUpdateDomain(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>> virNetworkUpdateBridge(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>> virNetworkUpdateIpDnsHost(virNetworkPtr network,
>> const char *xml,
>> unsigned int flags);
>> etc. (etc. etc.)
>>
>> "flags" would include:
>>
>> /* force add if object with matching key doesn't exist */
>> VIR_NETWORK_UPDATE_ADD
>> /* delete existing object(s) that matches the xml */
>> VIR_NETWORK_UPDATE_DELETE
>> /* take effect immediately */
>> VIR_NETWORK_UPDATE_LIVE
>> /* persistent change */
>> VIR_NETWORK_UPDATE_CONFIG
>>
>>
>> This method could be problematic, e.g., for something like
>> virNetworkUpdateIpDhcpHost() - although currently only a single <ip>
>> element can have a <dhcp> entries, in the future we could allow any/all
>> of them to have dhcp entries. Another downside is that we would need to
>> add an new function for each new element we decide we want to be able to
>> update.
> that's an awful lot of potential APIs something more generic is needed
>
>> ===========
>> OPTION 3) Again, a single API, but with an added "nodespec" arg which would
>> be used to describe the parent node you of the node you want added/updated to:
>>
>> int virNetworkUpdate(virNetworkPtr network,
>> const char *nodeSpec,
>> const char *xml,
>> unsigned int flags);
>>
>> For example, if you wanted to add a new <host> entry to <ip
>> address='10.24.75.1' ...>'s dhcp element, you would do this:
>>
>> /* nodespec obviously uses an XPath expression */
>> virNetworkUpdate("/ip[address='10.24.75.1']/dhcp",
>> /* full text of <host> element to add */
>> "<host mac='00:16:3e:77:e2:ed' "
>> "name='X.example.com' ip='10.24.75.10'/>"
>> VIR_NETWORK_UPDATE_ADD | VIR_NETWORK_UPDATE_LIVE
>> | VIR_NETWORK_UPDATE_CONFIG);
>>
>> Or, to change the contents of the exiting portgroup "engineering" you
>> would use:
>>
>> virNetworkUpdate("/",
>> /* full text of portgroup */,
>> "<portgroup name='engineering'> ..... </portgroup>"
>> VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG);
>>
>> To delete a static dhcp host entry, you would use this:
>>
>> virNetworkUpdate("/ip[address='10.24.75.1']/dhcp",
> shouldn't that be //ip[address='10.24.75.1']/dhcp or your really
> expect to have ip as the root ?
Keep in mind that I've only used xpath expressions as much as I've
needed them in libvirt's parsing functions, which usually use "./xxxxx"
style, so I probably got the syntax wrong.
I had figured that the root node would be <network>, and would be "/". I
*think* I've just learned from you that isn't the case, and that an <ip>
element within <network> would be specified (if network was the root
node) as "//ip".
>
>> /* just enough to match existing entry */
>> "<host mac='00:16:3e:77:e2:ed'/>",
>> VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE
>> | VIR_NETWORK_UPDATE_CONFIG);
>>
>> or if you preferred:
>>
>> virNetworkUpdate("/ip[address='10.24.75.1']/dhcp",
>> /* again, just enough to match */
>> "<host ip='10.24.75.10'/>",
>> VIR_NETWORK_UPDATE_DELETE |VIR_NETWORK_UPDATE_LIVE
>> | VIR_NETWORK_UPDATE_CONFIG);
>>
>> To remove an interface from the interface pool:
>>
>> virNetworkUpdate("/forward",
>> "<interface dev='eth20'/>",
>> VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE
>> | VIR_NETWORK_UPDATE_CONFIG)
>>
>> (adding an interface would be identical, except the flag).
>>
>> (An alternate implementation would be to have nodeSpec specify the node
>> that is being updated/removed rather than its parent. This may make more
>> logical sense, (although not for ADD) and would even make the rest of
>> the code simpler for update/remove (we wouldn't have to do a manual
>> search within the node).
>>
>> The positive side of this is that the one API function allows you to modify
>> *anything* (even stuff not yet invented, so it's future-proof). The negative
>> side is that the code that implements it would need to go to quite a bit of
>> extra trouble to determine what has been changed (basically, it would
>>
>> a) generate XML for the current state of the network,
>> b) parse that into an xmlNode,
>> c) perform the operation on xmlNode using nodeset to figure out where,
>> and adding in the new xml (or removing any nodes matching it),
>> d) convert modified xmlNode back to xml text,
>> e) parse the xml text into a virNetworkDef,
>> f) compare it to the original virNetworkDef to determine what to do.
> One problem I can see with 3) is that it would work for now
> but if we were to add namespaces to the XML then the API would
> not allow to address those elements/attributes
>
> //foo in XPath cannot address an element foo with a namespace
>
> to do this you need to do
>
> //prefix:foo and provide separately the mapping between prefix
> and the namespace URI
>
> So that will work as long as we don't add namespace to the XML
Hmm. I had been thinking recently of adding a private namespace to allow
adding custom commandline options to the dnsmasq commandline similar to
how we currently use a private namespace to allow adding custom
commandline options to the qemu commandline. Maybe something like this:
network xmlns:dnsmasq='http://libvirt.org/schemas/network/dnsmasq/1.0'>
<ip address='192.168.122.1' netmask='255.255.255.0'>
<dhcp>
<range start='10.24.75.128' end='10.24.75.254' />
<dnsmasq:commandline>
<dnsmasq:arg value='--max-ttl'/>
<dnsmasq:arg value='25400'/>
<dnsmasq:arg value='--resolv-file=/etc/libvirt-resolv.conf'/>
</dnsmasq:commandline>
</dhcp>
</ip>
</network>
So you're saying that xpath wouldn't allow me to select the
<dnsmasq:commandline> element and replace it? (my guess at the
expression to do that would have been
"//ip[address='10.24.75.128']/dhcp/dnsmasq::commandline")
> Another problem is making sure we have the addressing complete,
> for example suppose we have
> <
>
> The other problem is that people who already have a beef with XML usage
> will see red when they see the need to learn XPath O:-) , but that could
> be alleviated in a large part with a good set of examples !
>
> Also the problem with Update APIs to XML is that the next step is
> people will want to add one part (not replace) and then you need
> to define where to add, before/after ...
Fortunately there is nothing in <network> that is order-dependent, but
in the general sense I understand the problem you're describing. I think
what I'm taking away from your comments is that this isn't the 100%
future-proof solution that I'd imagined.
>
> XML editing APIs are *hard*, I'm actually a bit worried to go there
> something like 2/ while potentially leading to more entry points
> probably makes thing easier for the user.
Or maybe I should reconsider option (1) (a single API that just replaces
the entire XML for the network). That would also require special
considerations for fine-grained access control though - it would need to
compare the old and new, then check to see if the current user was
authorized to change the bits that showed up in the diff. Again, I don't
know how well (if at all) that fits in with the model danpb is planning
to use for that - if it's a model where user's are granted access purely
on a per-API basis, then it won't work. If it's possible to do the
finer-grained check further down in the implementation of the API, then
it might work.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list