10:51 a.m.
On 09/06/2012 04:52 PM, Eric Blake wrote:
On 09/06/2012 07:43 AM, Daniel P. Berrange wrote:
>> ===========
>> 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:
>>
>> 3) Can you think of a situation where this wouldn't work?
> In a libvirt API where you don't use XML directly.
>
>
> The thing I like about option 3 is that you are just passing in little
> snippets of XML to either add/remove/modify. But we need to figure out
> a better way todo this, without using XPath markers IMHO.
Option 4
Instead of an arbitrary XPath marker, can we come up with an enum of
possible change sites? That would mean the user doesn't have quite as
much flexibility (passing an int instead of string), but we are still
extensible as new changeable sites make sense (add another enum
constant). There's still some disambiguation needed, but I suppose you
could obtain that by passing (a subset of) the original XML sufficient
to provide the context necessary.
Yeah, this is more or less what we discussed separately on Wednesday,
and seems to fit with both Dan and DV's ideas.
For example,
/* full text of <host> element to add to <dhcp>, embedded in
* enough partial context from the original xml as to find the
* right <dhcp> to add it to */
virNetworkUpdate(VIR_NETWORK_UPDATE_DHCP,
"<network>"
" <ip address='10.24.75.1'>"
" <dhcp>"
" <host mac='00:16:3e:77:e2:ed' "
" name='X.example.com' ip='10.24.75.10'/>"
" </dhcp>"
" </ip>"
"</network>",
I wouldn't include the whole hierarchy, though - just the <host>
element. (instead of specifying adress='10.24.75.1' to figure out which
ip address, we could use DV's suggestion of including a simple "index"
argument). This may be slightly less flexible, but removes the need to
deal with an XML hierarchy, and works for current needs.
So the new API would be this:
int virNetworkUpdate(virNetworkPtr network,
unsigned int section,
size_t index,
char *xml,
unsigned int flags);
section would be one of the following:
typedef enum virNetworkUpdateSection {
VIR_NETWORK_SECTION_BRIDGE = 0,
VIR_NETWORK_SECTION_DOMAIN = 1,
VIR_NETWORK_SECTION_IP = 2,
VIR_NETWORK_SECTION_IP_DHCP_HOST = 3,
VIR_NETWORK_SECTION_IP_DHCP_RANGE = 4,
VIR_NETWORK_SECTION_FORWARD = 5,
VIR_NETWORK_SECTION_FORWARD_INTERFACE = 6,
VIR_NETWORK_SECTION_FORWARD_PF = 7,
VIR_NETWORK_SECTION_PORTGROUP = 8,
VIR_NETWORK_SECTION_DNS_HOST = 9,
VIR_NETWORK_SECTION_DNS_TXT = 10,
VIR_NETWORK_SECTION_DNS_SRV = 11,
};
In each case, the xml would need to be a full element named by the final
bit of the enum (so a bit of redundancy, but I think necessary for
consistency), and generally (with the exception of portgroup and dns
host) would not have any subelements (this both makes it simpler to
understand/translate into libvirt-glib, and limits the impact); for example:
virNetworkUpdate(net, VIR_NETWORK_SECTION_BRIDGE, 0
"<bridge name='br0' stp='off'/>",
VIR_NETWORK_UPDATE_LIVE|VIR_NETWORK_UPDATE_CONFIG);
(this would modify the existing <bridge> element (presumably to turn off
stp); since UPDATE_ADD wasn't specified, if there was no bridge element
already present, this would fail). Note the unused "index" of 0.
virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0,
"<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);
(adds a new host. If any of the attributes match an existing host, it
would fail).
The index is still 0 - it could be changed to "1" if we wanted to change
a host entry in the dhcp of the network's 2nd ip address. (Should index
instead be a const char *, allowing for character indexes, like
"10.24.75.1"? Currently it's only there to allow for future expansion;
maybe a single integer doesn't allow for enough expansion, but a char*
would...) (or should I drop the whole idea of the index, and just
include it in a separate xml element at the beginning of the xml arg
when it becomes necessary?). It's important to notice that the index
isn't within the dhcp's host array, but within the parent IPs. This does
point out a possible deficiency with the idea of having a single index -
in the future there may be a case where multiple indexes are required.
Deleting a host would be done like this:
virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0,
/* 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(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0,
/* 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(net, VIR_NETWORK_SECTION_FORWARD_INTERFACE, 0,
"<interface dev='eth20'/>",
VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE
| VIR_NETWORK_UPDATE_CONFIG)
To modify the portgroup named "engineering" (e.g. to increase the
inbound average bandwidth from 1000 to 2000):
virNetworkUpdate(net, VIR_NETWORK_SECTION_PORTGROUP, 0,
"<portgroup name='engineering'
default='yes'>"
" <virtualport type='802.1Qbh'>"
" <parameters profileid='test'/>"
" </virtualport>"
" <bandwidth>"
" <inbound average='2000' peak='5000'
burst='5120'/>"
" <outbound average='1000' peak='5000'
burst='5120'/>"
" </bandwidth>"
"</portgroup>",
VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE
| VIR_NETWORK_UPDATE_CONFIG)
So how does this sound? Am I getting closer? Or further away?