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 :)
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list