Re: [libvirt] [PATCHv2 3/7] interface: implement public APIs for libvirt transactional network changes
On 05/24/2011 04:44 AM, Daniel P. Berrange wrote:
On Tue, May 24, 2011 at 02:13:12PM +0800, Daniel Veillard wrote:
> On Thu, May 19, 2011 at 04:51:25PM -0400, Laine Stump wrote:
>> From: Michal Privoznik<mprivozn(a)redhat.com>
>>
>> ---
>> src/libvirt.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 139 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index ff16c48..786ce15 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -8143,7 +8143,9 @@ error:
>> * @xml: the XML description for the interface, preferably in UTF-8
>> * @flags: and OR'ed set of extraction flags, not used yet
>> *
>> - * Define an interface (or modify existing interface configuration)
>> + * Define an interface (or modify existing interface configuration).
>> + * This can, however be affected by virInterfaceChangeBegin
>> + * and/or friends.
> Small nit, I would suggest making the 4 extra comments a bit more
> explicit that it's about transaction support, something along the
> lines of:
>
> * This can however be affected by the transaction support for
> * interface configuration change, see virInterfaceChangeBegin(),
> * virInterfaceChangeCommit() and related functions.
I made the comments more verbose (possibly *too* verbose) in the
upcoming V3 of this patch. If the explanation is too much, you can let
me know and I'll dial it back before pushing.
> [...]
>> @@ -8374,6 +8382,135 @@ virInterfaceFree(virInterfacePtr iface)
>> return 0;
>> }
>>
>> +/**
>> + * virInterfaceChangeBegin:
>> + * @conn: pointer to hypervisor connection
>> + * @flags: flags, not used yet
>> + *
>> + * This functions creates a restore point to which one can return
>> + * later by calling virInterfaceChangeRollback. This function should
>> + * be called before any transaction with interface configuration.
>> + * Once knowing, new configuration works, it can be commited via
>> + * virInterfaceChangeCommit, which frees restore point.
> which frees the restore point.
>
> I would like the description of what happens ona sequence of
> virInterfaceChangeBegin()
> virInterfaceChangeBegin()
>
> calls for the same connection, is that an error ? Will netcf implement
> this, this behaviour should probably be documented.
It should return the VIR_ERR_INVALID_OPERATION code with
a message that a transaction is already active.
Yes, netcf checks for this and returns an error (likewise if commit or
rollback is called when there is no open transaction). I will make sure
this is translated to VIR_ERR_INVALID_OPERATION so that it is logged
properly.
(it's good that you pointed this out now, because netcf had been just
returning a generic error for this, so I modified the netcf patches to
return a new unique code, and have updated patch 6/7 (the netcf driver
additions) to recognize this new code when it's available).
>> +/**
>> + * virInterfaceChangeCommit:
>> + * @conn: pointer to hypervisor connection
>> + * @flags: flags, not used yet
>> + *
>> + * This commits the changes made to interfaces and frees restore point
> "frees the restore point"
>
>> + * created by virInterfaceChangeBegin.
> Same thing behaviour of virInterfaceChangeCommit() in case there was
> no Begin() associated should be documented.
Likewise VIR_ERR_INVALID_OPERATION
Yup.