On Fri, Sep 07, 2012 at 11:51:39AM -0400, Laine Stump wrote:
> 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?
That would work for me. My only point of concern is index,
and maybe behaviour of edge cases.
If I tweak your first example:
virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0,
/* just enough to match existing entry */
"<host/>",
VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE
| VIR_NETWORK_UPDATE_CONFIG);
This may match multiple entries, do we preserve order of those entries ?
If not that mean deleting one of the host at random. Is that okay ? I
would think yes. Would that be equivalent to
virNetworkUpdate(net, VIR_NETWORK_SECTION_IP_DHCP_HOST, 0,
NULL,
VIR_NETWORK_UPDATE_DELETE | VIR_NETWORK_UPDATE_LIVE
| VIR_NETWORK_UPDATE_CONFIG);
I would assume yes in practice, the selector would be enough to tell
what kind of inforamtions we are trying to remove.
I guess looking at some early patch on how the matching is to be done
based on the information given by the users, will tell us if that
approach is a nightmare or ends up being decent. If trying to lookup the
information based on the xml turns horrible, then maybe fallback
to XPath (and make clear that we won't support namespaces)
I would assume that there are multiple VIR_NETWORK_UPDATE_... flags
VIR_NETWORK_UPDATE_DELETE
VIR_NETWORK_UPDATE_ADD_LAST
VIR_NETWORK_UPDATE_ADD_FIRST
VIR_NETWORK_UPDATE_EXISTING
i.e. don't try to infer from the matching whether it is an addition or a
modification.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list