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

[Laine Stump]

On 09/17/2012 06:42 PM, Eric Blake wrote:
> 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?

I like that idea! I'll be posting a V2 in a bit, and that will be one of
the changes.

>
>> +/*
>> + * 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?


Yes.


>> +++ 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)?


I'll try to put in better documentation in the next round.


>   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)?

Actually, yes. It ended up the way it is just because the arguments grew
organically from my virNetworkDefineFlags idea. But when you compare
what's there now with an API that has a separate "command" arg, this
version is no more compact in the source, and one more unsigned int arg
is no big deal.

So, before I move on to the virsh command (I finished the first backend
section implementation a little while ago), I'll do another pass through
git rebase -i, adding in the extra arg.

> 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 :)
>

Actually, when I added that line, I was thinking more about the RPC
numbers and how the latest one is always last, for some reason. I guess
that's what happens when you're running on fumes...