Re: [libvirt] [PATCH 03/10] network: define new API virNetworkUpdate

On 09/17/2012 03:48 AM, Laine Stump wrote: > This patch adds a new public API virNetworkUpdate that will permit > updating an existing network configuration without requiring that the > network be destroyed/restarted for the changes to take effect. > --- > include/libvirt/libvirt.h.in | 50 +++++++++++++++++++++++++++++++++++++ > src/driver.h | 7 ++++++ > src/libvirt.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ > src/libvirt_public.syms | 1 + > 4 files changed, 117 insertions(+) This one is the clincher - either we take this API as written for the 0.10.2 freeze tomorrow (guaranteeing it will be in the .so, even if the implementation is still baking), or we wait another release cycle to come up with something better (and given the churn we have already been through, I don't know that we'll have any breakthroughs for a nicer API). Dan's already given a weak ACK to the API, and I will also throw in my assent - this is better than any of the other 3 earlier designs we tried (reusing the define XML to redefine, but no way to limit how much was redefined; dealing with an explosion in new APIs; or exposing XPath selections to the user); it is at least extensible, and your cover letter demonstrated several valid use cases where this was sufficient for the task. You may want to wait on DV's opinion before pushing (or DV may want to push before cutting the rc1 release if he's online when you aren't - that would also serve as consent). Also, although I'm agreeing to the API, I have some review comments that might warrant a v2. As for the rest of the series, I'm okay if you wait for the review of all of the patches as a unit before pushing any implementation, even if that means missing the rc1 freeze. Once the API is in place, we can then treat the rest of the series as a bug fix to an incomplete API as justification for including it in rc2 if we miss rc1. Of course, the sooner it is in, the more testing we can give it. Not to mention I already noticed you posted a v2 on patch 6/10, so like me, you are in the same boat of trying to get implementation to match the interface. > +++ b/include/libvirt/libvirt.h.in > @@ -2328,6 +2328,56 @@ virNetworkPtr virNetworkDefineXML (virConnectPtr conn, > int virNetworkUndefine (virNetworkPtr network); > > /* > + * virNetworkUpdateSection: > + * > + * describes which section of a <network> definition the provided > + * xml should be applied to. > + * > + */ > +typedef enum { > + VIR_NETWORK_SECTION_BRIDGE = 0, I'm fine with this as-is, but maybe we want to start at 1, to ensure that the user is always passing in a section number and not blindly passing 0 without realizing how to use the API?

> +/* > + * virNetworkUpdateFlags: > + * > + * Used to specify what type of operation to perform, and whether to > + * affect live network, persistent config, or both. > + */ > +typedef enum { > + VIR_NETWORK_UPDATE_AFFECT_CURRENT = 0, > + VIR_NETWORK_UPDATE_AFFECT_LIVE = 1 << 0, > + VIR_NETWORK_UPDATE_AFFECT_CONFIG = 1 << 1, > + VIR_NETWORK_UPDATE_EXISTING = 1 << 2, > + VIR_NETWORK_UPDATE_DELETE = 1 << 3, > + VIR_NETWORK_UPDATE_ADD_LAST = 1 << 4, > + VIR_NETWORK_UPDATE_ADD_FIRST = 1 << 5, Am I correct that EXISTING, DELETE, ADD_LAST, and ADD_FIRST are mutually exclusive, and that exactly one has to be provided?

> +++ b/src/libvirt.c > @@ -10407,6 +10407,65 @@ error: > } > > /** > + * virNetworkUpdate: > + * @network: pointer to a defined network > + * @section: which section of the network to update > + * (virNetworkUpdateSection) > + * @parentIndex: which parent element, if there are multiples parents s/multiples/multiple/ > + * of the same type (e.g. which <ip> element when modifying > + * a <dhcp>/<host> element), or "-1" for "don't care" or > + * "automatically find appropriate one". > + * @xml: the XML description for the network, preferably in UTF-8 > + * @flags: bitwise OR of virNetworkUpdateFlags. > + * > + * Update the definition of an existing network, either its live > + * running state, its persistent configuration, or both. > + * > + * Returns 0 in case of success, -1 in case of error Where do you document the four modes (existing, delete, add_first, add_last)?

Since the modes are mutually exclusive, is it worth passing them in as a separate argument (with values 0, 1, 2, 3), instead of as bits within flags (with values 4, 8, 16, 32)?

But adding a new argument would cause you more rebase work, so I can live with them as flags for now. Another possibility might be passing them as 2-bit combinations within the flags parameter (that will automatically enforce the mutual exclusion as well as necessarily supplying exactly one of the four choices), by encoding: VIR_NETWORK_UPDATE_ACTION_MASK = 0xc, VIR_NETWORK_UPDATE_EXISTING = 0x0, VIR_NETWORK_UPDATE_DELETE = 0x4, VIR_NETWORK_UPDATE_ADD_LAST = 0x8, VIR_NETWORK_UPDATE_ADD_FIRST = 0xc, > + */ > +int > +virNetworkUpdate(virNetworkPtr network, > + unsigned int section, /* virNetworkUpdateSection */ > + int parentIndex, > + const char *xml, > + unsigned int flags) > +{ > + virConnectPtr conn; > + VIR_DEBUG("network=%p, section=%d, parentIndex=%d, xml=%s, flags=0x%x", > + network, section, parentIndex, xml, flags); > + > + virResetLastError(); > + > + if (!VIR_IS_CONNECTED_NETWORK(network)) { > + virLibNetworkError(VIR_ERR_INVALID_NETWORK, __FUNCTION__); > + virDispatchError(NULL); > + return -1; > + } > + conn = network->conn; > + if (conn->flags & VIR_CONNECT_RO) { > + virLibNetworkError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); > + goto error; > + } > + > + virCheckNonNullArgGoto(xml, error); If you _don't_ encode the four modes as a separate argument or as a two-bit field within flags, then it might be worth enforcing here at the API entry point that exactly one of the four flags is always specified, as in: if ((!!(flags & _EXISTING) + !!(flags & _DELETE) + !!(flags & _ADD_FIRST) + !!(flags & _ADD_LAST)) != 1) raise error; But this is bike-shedding - whether or not we error out on invalid mode combinations can be viewed as implementation, not interface, and therefore something we could add later if we want to take this commit as-is for the interface. > +++ b/src/libvirt_public.syms > @@ -565,6 +565,7 @@ LIBVIRT_0.10.2 { > virNodeGetMemoryParameters; > virNodeSetMemoryParameters; > virStoragePoolListAllVolumes; > + virNetworkUpdate; You probably guessed I would catch this, but I'll say it anyways :) Sorting :)