From: Michal Privoznik <mprivozn(a)redhat.com&gt; This is the API agreed on in: https://www.redhat.com/archives/libvir-list/2011-May/msg00026.html (with a slight name change to use "...begin" rather than "...start"). This implements transactional changes to the host network config. When a transaction is begun with ncf_change_begin(), all other netcf APIs will continue to work as they always have, but a snapshot of the existing config will be taken, allowing reversion (rollback, using ncf_change_rollback()) to the exact state of config at the time ncf_change_begin() was called. Alternately, if it's determined that the new changes are acceptable, ncf_change_commit() can be called, which will eliminate the snapshot and make the changes permanent. As a failsafe measure, if neither ncf_change_commit() or ncf_change_rollback() is called by the next time the system reboots, the netcf-transaction initscript will be automatically called to rollback the changes. --- include/libvirt/libvirt.h.in | 7 +++++++ src/libvirt_public.syms | 3 +++ 2 files changed, 10 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7cd6e13..4a45390 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1405,6 +1405,13 @@ int virInterfaceDestroy (virInterfacePtr iface, int virInterfaceRef (virInterfacePtr iface); int virInterfaceFree (virInterfacePtr iface); +int virInterfaceChangeBegin (virConnectPtr conn, + unsigned int flags); +int virInterfaceChangeCommit (virConnectPtr conn, + unsigned int flags); +int virInterfaceChangeRollback(virConnectPtr conn, + unsigned int flags); + /** * virStoragePool: * diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 0590535..f4f8262 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -442,6 +442,9 @@ LIBVIRT_0.9.2 { virDomainInjectNMI; virDomainScreenshot; virDomainSetSchedulerParametersFlags; + virInterfaceChangeBegin; + virInterfaceChangeCommit; + virInterfaceChangeRollback; } LIBVIRT_0.9.0; # .... define new API here using predicted next version number .... -- 1.7.3.4

From: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/driver.h | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index 450dd53..c9747aa 100644 --- a/src/driver.h +++ b/src/driver.h @@ -865,6 +865,15 @@ typedef int typedef int (*virDrvInterfaceIsActive)(virInterfacePtr iface); +typedef int + (*virDrvInterfaceChangeBegin) (virConnectPtr conn, + unsigned int flags); +typedef int + (*virDrvInterfaceChangeCommit) (virConnectPtr conn, + unsigned int flags); +typedef int + (*virDrvInterfaceChangeRollback)(virConnectPtr conn, + unsigned int flags); typedef struct _virInterfaceDriver virInterfaceDriver; typedef virInterfaceDriver *virInterfaceDriverPtr; @@ -895,6 +904,9 @@ struct _virInterfaceDriver { virDrvInterfaceCreate interfaceCreate; virDrvInterfaceDestroy interfaceDestroy; virDrvInterfaceIsActive interfaceIsActive; + virDrvInterfaceChangeBegin interfaceChangeBegin; + virDrvInterfaceChangeCommit interfaceChangeCommit; + virDrvInterfaceChangeRollback interfaceChangeRollback; }; -- 1.7.3.4

From: Michal Privoznik <mprivozn(a)redhat.com&gt; --- 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. * * Returns NULL in case of error, a pointer to the interface otherwise */ @@ -8189,6 +8191,8 @@ error: * * Undefine an interface, ie remove it from the config. * This does not free the associated virInterfacePtr object. + * This can, however be affected by virInterfaceChangeBegin + * and/or friends. * * Returns 0 in case of success, -1 in case of error */ @@ -8230,7 +8234,9 @@ error: * @iface: pointer to a defined interface * @flags: and OR'ed set of extraction flags, not used yet * - * Activate an interface (ie call "ifup") + * Activate an interface (ie call "ifup"). + * This can, however be affected by virInterfaceChangeBegin + * and/or friends. * * Returns 0 in case of success, -1 in case of error */ @@ -8276,6 +8282,8 @@ error: * deactivate an interface (ie call "ifdown") * This does not remove the interface from the config, and * does not free the associated virInterfacePtr object. + * This can, however be affected by virInterfaceChangeBegin + * and/or friends. * * Returns 0 in case of success and -1 in case of failure. */ @@ -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. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeBegin(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver && conn->interfaceDriver->interfaceChangeBegin) { + ret = conn->interfaceDriver->interfaceChangeBegin(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} + +/** + * virInterfaceChangeCommit: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This commits the changes made to interfaces and frees restore point + * created by virInterfaceChangeBegin. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeCommit(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver && conn->interfaceDriver->interfaceChangeCommit) { + ret = conn->interfaceDriver->interfaceChangeCommit(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} + +/** + * virInterfaceChangeRollback: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This cancels changes made to interfaces settings by restoring previous + * state created by virInterfaceChangeBegin. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeRollback(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver && + conn->interfaceDriver->interfaceChangeRollback) { + ret = conn->interfaceDriver->interfaceChangeRollback(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} /** * virStoragePoolGetConnect: -- 1.7.3.4

On Tue, May 24, 2011 at 02:13:12PM +0800, Daniel Veillard wrote: It should return the VIR_ERR_INVALID_OPERATION code with a message that a transaction is already active. Likewise VIR_ERR_INVALID_OPERATION Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

From: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt.c | 178 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 176 insertions(+), 2 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index ff16c48..12b849a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -8143,7 +8143,18 @@ 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). + * + * Normally this change in the interface configuration is immediately + * permanent/persistent, but if virInterfaceChangeBegin() has been + * previously called (i.e. if an interface config transaction is + * open), the new interface definition will only become permanent if + * virInterfaceChangeCommit() is called prior to the next reboot of + * the system running libvirtd. Prior to that time, it can be + * explicitly removed using virInterfaceChangeRollback(), or will be + * automatically removed during the next reboot of the system running + * libvirtd. * * Returns NULL in case of error, a pointer to the interface otherwise */ @@ -8190,6 +8201,16 @@ error: * Undefine an interface, ie remove it from the config. * This does not free the associated virInterfacePtr object. * + * Normally this change in the interface configuration is + * permanent/persistent, but if virInterfaceChangeBegin() has been + * previously called (i.e. if an interface config transaction is + * open), the removal of the interface definition will only become + * permanent if virInterfaceChangeCommit() is called prior to the next + * reboot of the system running libvirtd. Prior to that time, the + * definition can be explicitly restored using + * virInterfaceChangeRollback(), or will be automatically restored + * during the next reboot of the system running libvirtd. + * * Returns 0 in case of success, -1 in case of error */ int @@ -8230,7 +8251,12 @@ error: * @iface: pointer to a defined interface * @flags: and OR'ed set of extraction flags, not used yet * - * Activate an interface (ie call "ifup") + * Activate an interface (i.e. call "ifup"). + * + * If there was an open network config transaction at the time this + * interface was defined (that is, if virInterfaceChangeBegin() had + * been called), the interface will be brought back down (and then + * undefined) if virInterfaceChangeRollback() is called. * * Returns 0 in case of success, -1 in case of error */ @@ -8277,6 +8303,13 @@ error: * This does not remove the interface from the config, and * does not free the associated virInterfacePtr object. * + + * If there is an open network config transaction at the time this + * interface is destroyed (that is, if virInterfaceChangeBegin() had + * been called), and if the interface is later undefined and then + * virInterfaceChangeRollback() is called, the restoral of the + * interface definition will also bring the interface back up. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -8374,6 +8407,147 @@ 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 the restore point. + * + * If virInterfaceChangeBegin() is called when a transaction is + * already opened, this function will fail, and a + * VIR_ERR_INVALID_OPERATION will be logged. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeBegin(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver && conn->interfaceDriver->interfaceChangeBegin) { + ret = conn->interfaceDriver->interfaceChangeBegin(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} + +/** + * virInterfaceChangeCommit: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This commits the changes made to interfaces and frees the restore point + * created by virInterfaceChangeBegin(). + * + * If virInterfaceChangeCommit() is called when a transaction is not + * opened, this function will fail, and a VIR_ERR_INVALID_OPERATION + * will be logged. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeCommit(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver && conn->interfaceDriver->interfaceChangeCommit) { + ret = conn->interfaceDriver->interfaceChangeCommit(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} + +/** + * virInterfaceChangeRollback: + * @conn: pointer to hypervisor connection + * @flags: flags, not used yet + * + * This cancels changes made to interfaces settings by restoring previous + * state created by virInterfaceChangeBegin(). + * + * If virInterfaceChangeRollback() is called when a transaction is not + * opened, this function will fail, and a VIR_ERR_INVALID_OPERATION + * will be logged. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virInterfaceChangeRollback(virConnectPtr conn, unsigned int flags) +{ + int ret = -1; + + VIR_DEBUG("conn=%p, flags=%d", conn, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + goto end; + } + + if (conn->flags & VIR_CONNECT_RO) { + virLibInterfaceError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + virDispatchError(conn); + goto end; + } + + if (conn->interfaceDriver && + conn->interfaceDriver->interfaceChangeRollback) { + ret = conn->interfaceDriver->interfaceChangeRollback(conn, flags); + } else { + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + } + +end: + VIR_DEBUG("returning: %d", ret); + return ret; +} /** * virStoragePoolGetConnect: -- 1.7.3.4

On Thu, May 19, 2011 at 04:51:25PM -0400, Laine Stump wrote: The code pattern / flow control in these 3 APIs is completely different from all the other public APIs, for no obvious benefit. It needs to be updated to follow the same code pattern as the existing APIs impls. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Thu, May 19, 2011 at 04:51:26PM -0400, Laine Stump wrote: Looks fine, the cleanup on remote stub generation really make those remote support patches way nicer :-) ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, May 19, 2011 at 04:51:27PM -0400, Laine Stump wrote: ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Thu, May 19, 2011 at 04:51:28PM -0400, Laine Stump wrote: ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 05/26/2011 04:35 AM, Daniel P. Berrange wrote: Oops. yeah, fixed. Okay, I've changed that in these three new methods, and will submit a followup patch to change it in the existing methods as well..

def) Based on the check above I assume the driver can decide whether to accept/support nested transactions (ie, this test driver does not support them for example, because it uses a boolean variable to remember its state). Is it correct? (If not, maybe you can move this check one layer above, before the driver) /Chris {

On 05/24/2011 02:24 AM, Daniel Veillard wrote: But virInterfaceObjListFree(dest) doesn't free dest itself, it only frees the items that are contained by dest (not dest itself), and those items were not allocated by the caller, but were allocated within virInterfaceObjListClone(), so I don't see the problem you see. There *is* a problem in general with this function, though - dest is not cleaned out before copying over the contents of src, so if it starts out non-empty, there may be extra stuff in there at the end. Michal - since you wrote it, maybe you should answer this question - shouldn't virInterfaceObjListClone() call virInterfaceObjListFree(dest) as soon as it determines dest != NULL?