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@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.

[...]

...

@@ -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.

...

+/** + * 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 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@redhat.com> --- 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:

From: Michal Privoznik <mprivozn@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. * * 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; +}

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:

From: Michal Privoznik <mprivozn@redhat.com>

--- src/remote/remote_driver.c | 3 +++ src/remote/remote_protocol.x | 17 ++++++++++++++++- src/remote_protocol-structs | 9 +++++++++ 3 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 8c69743..5d837d1 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6909,6 +6909,9 @@ static virInterfaceDriver interface_driver = { .interfaceCreate = remoteInterfaceCreate, /* 0.7.2 */ .interfaceDestroy = remoteInterfaceDestroy, /* 0.7.2 */ .interfaceIsActive = remoteInterfaceIsActive, /* 0.7.3 */ + .interfaceChangeBegin = remoteInterfaceChangeBegin, /* 0.9.2 */ + .interfaceChangeCommit = remoteInterfaceChangeCommit, /* 0.9.2 */ + .interfaceChangeRollback = remoteInterfaceChangeRollback, /* 0.9.2 */ };

static virStorageDriver storage_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index c784dba..12fd395 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1164,6 +1164,18 @@ struct remote_interface_destroy_args { unsigned int flags; };

+struct remote_interface_change_begin_args { + unsigned int flags; +}; + +struct remote_interface_change_commit_args { + unsigned int flags; +}; + +struct remote_interface_change_rollback_args { + unsigned int flags; +}; +

/* Auth calls: */

@@ -2291,8 +2303,11 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_MIGRATE_PERFORM3 = 216, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_MIGRATE_FINISH3 = 217, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_MIGRATE_CONFIRM3 = 218, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_SET_SCHEDULER_PARAMETERS_FLAGS = 219 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_SET_SCHEDULER_PARAMETERS_FLAGS = 219, /* skipgen skipgen */ + REMOTE_PROC_INTERFACE_CHANGE_BEGIN = 220, /* autogen autogen */

+ REMOTE_PROC_INTERFACE_CHANGE_COMMIT = 221, /* autogen autogen */ + REMOTE_PROC_INTERFACE_CHANGE_ROLLBACK = 222 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? * Nice isn't it. Please keep it this way when adding more. diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 414b4d5..a28cd13 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -803,6 +803,15 @@ struct remote_interface_destroy_args { remote_nonnull_interface iface; u_int flags; }; +struct remote_interface_change_begin_args { + unsigned int flags; +}; +struct remote_interface_change_commit_args { + unsigned int flags; +}; +struct remote_interface_change_rollback_args { + unsigned int flags; +}; struct remote_auth_list_ret { struct { u_int types_len;

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

From: Michal Privoznik <mprivozn@redhat.com>

This implements the commands iface-begin, iface-commit, and iface-rollback, which simply call the corresponding functions in the libvirt API. --- tools/virsh.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 103 insertions(+), 0 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index c2f4de6..6d72c75 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5118,6 +5118,103 @@ cmdInterfaceDestroy(vshControl *ctl, const vshCmd *cmd) return ret; }

+/* + * "iface-begin" command + */ +static const vshCmdInfo info_interface_begin[] = { + {"help", N_("create a snapshot of current interfaces settings, " + "which can be later commited (iface-commit) or " + "restored (iface-rollback)")}, + {"desc", N_("Create a restore point for interfaces settings")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_begin[] = { + {NULL, 0, 0, NULL} +}; + +static bool +cmdInterfaceBegin(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + bool ret = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto end; + + if (virInterfaceChangeBegin(ctl->conn, 0) < 0) { + vshError(ctl, "%s", _("Failed to begin network config change transaction")); + } else { + vshPrint(ctl, "%s", _("Network config change transaction started\n")); + ret = true; + } + +end: + return ret; +} + +/* + * "iface-commit" command + */ +static const vshCmdInfo info_interface_commit[] = { + {"help", N_("commit changes made since iface-begin and free restore point")}, + {"desc", N_("commit changes and free restore point")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_commit[] = { + {NULL, 0, 0, NULL} +}; + +static bool +cmdInterfaceCommit(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + bool ret = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto end; + + if (virInterfaceChangeCommit(ctl->conn, 0) < 0) { + vshError(ctl, "%s", _("Failed to commit network config change transaction")); + } else { + vshPrint(ctl, "%s", _("Network config change transaction committed\n")); + ret = true; + } + +end: + return ret; +} + +/* + * "iface-rollback" command + */ +static const vshCmdInfo info_interface_rollback[] = { + {"help", N_("rollback to previous saved configuration created via iface-begin")}, + {"desc", N_("rollback to previous restore point")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_interface_rollback[] = { + {NULL, 0, 0, NULL} +}; + +static bool +cmdInterfaceRollback(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + bool ret = false; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto end; + + if (virInterfaceChangeRollback(ctl->conn, 0) < 0) { + vshError(ctl, "%s", _("Failed to rollback network config change transaction")); + } else { + vshPrint(ctl, "%s", _("Network config change transaction rolled back\n")); + ret = true; + } + +end: + return ret; +}

/* * "nwfilter-define" command @@ -10933,6 +11030,12 @@ static const vshCmdDef ifaceCmds[] = { info_interface_start, 0}, {"iface-undefine", cmdInterfaceUndefine, opts_interface_undefine, info_interface_undefine, 0}, + {"iface-begin", cmdInterfaceBegin, opts_interface_begin, + info_interface_begin, 0}, + {"iface-commit", cmdInterfaceCommit, opts_interface_commit, + info_interface_commit, 0}, + {"iface-rollback", cmdInterfaceRollback, opts_interface_rollback, + info_interface_rollback, 0}, {NULL, NULL, NULL, NULL, 0} };

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@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:

From: Michal Privoznik <mprivozn@redhat.com>

This is the functionality at the end of the libvirt part of the call chain - for each function, the corresponding netcf API is called. --- configure.ac | 5 +++ src/interface/netcf_driver.c | 76 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac index 8db3226..dbec54c 100644 --- a/configure.ac +++ b/configure.ac @@ -1491,6 +1491,11 @@ if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then if test "$with_netcf" = "yes" ; then AC_DEFINE_UNQUOTED([WITH_NETCF], 1, [whether libnetcf is available to configure physical host network interfaces]) + AC_CHECK_LIB([netcf], [ncf_change_begin], [netcf_transactions=1], [netcf_transactions=0]) + if test "$netcf_transactions" = "1" ; then + AC_DEFINE_UNQUOTED([HAVE_NETCF_TRANSACTIONS], ["1"], + [we have sufficiently new version of netcf for transaction network API]) + fi fi fi AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c index d5b401a..c94cbd5 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/netcf_driver.c @@ -540,6 +540,77 @@ cleanup: return ret; }

+#ifdef HAVE_NETCF_TRANSACTIONS +static int interfaceChangeBegin(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct interface_driver *driver = conn->interfacePrivateData; + int ret; + + virCheckFlags(0, -1); /* currently flags must be 0 */ + + interfaceDriverLock(driver); + + ret = ncf_change_begin(driver->netcf, 0); + if (ret < 0) { + const char *errmsg, *details; + int errcode = ncf_error(driver->netcf, &errmsg, &details); + interfaceReportError(netcf_to_vir_err(errcode), + _("failed to begin transaction (netcf: %s - %s)"), + errmsg, details ? details : ""); + } + + interfaceDriverUnlock(driver); + return ret; +} + +static int interfaceChangeCommit(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct interface_driver *driver = conn->interfacePrivateData; + int ret; + + virCheckFlags(0, -1); /* currently flags must be 0 */ + + interfaceDriverLock(driver); + + ret = ncf_change_commit(driver->netcf, 0); + if (ret < 0) { + const char *errmsg, *details; + int errcode = ncf_error(driver->netcf, &errmsg, &details); + interfaceReportError(netcf_to_vir_err(errcode), + _("failed to begin transaction (netcf: %s - %s)"), + errmsg, details ? details : ""); + } + + interfaceDriverUnlock(driver); + return ret; +} + +static int interfaceChangeRollback(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct interface_driver *driver = conn->interfacePrivateData; + int ret; + + virCheckFlags(0, -1); /* currently flags must be 0 */ + + interfaceDriverLock(driver); + + ret = ncf_change_rollback(driver->netcf, 0); + if (ret < 0) { + const char *errmsg, *details; + int errcode = ncf_error(driver->netcf, &errmsg, &details); + interfaceReportError(netcf_to_vir_err(errcode), + _("failed to begin transaction (netcf: %s - %s)"), + errmsg, details ? details : ""); + } + + interfaceDriverUnlock(driver); + return ret; +} +#endif /* HAVE_NETCF_TRANSACTIONS */ + static virInterfaceDriver interfaceDriver = { "Interface", .open = interfaceOpenInterface, /* 0.7.0 */ @@ -556,6 +627,11 @@ static virInterfaceDriver interfaceDriver = { .interfaceCreate = interfaceCreate, /* 0.7.0 */ .interfaceDestroy = interfaceDestroy, /* 0.7.0 */ .interfaceIsActive = interfaceIsActive, /* 0.7.3 */ +#ifdef HAVE_NETCF_TRANSACTIONS + .interfaceChangeBegin = interfaceChangeBegin, /* 0.9.2 */ + .interfaceChangeCommit = interfaceChangeCommit, /* 0.9.2 */ + .interfaceChangeRollback = interfaceChangeRollback, /* 0.9.2 */ +#endif /* HAVE_NETCF_TRANSACTIONS */ };

ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@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:

On Thu, May 19, 2011 at 04:51:28PM -0400, Laine Stump wrote:

...

From: Michal Privoznik<mprivozn@redhat.com>

This is the functionality at the end of the libvirt part of the call chain - for each function, the corresponding netcf API is called. --- configure.ac | 5 +++ src/interface/netcf_driver.c | 76 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac index 8db3226..dbec54c 100644 --- a/configure.ac +++ b/configure.ac @@ -1491,6 +1491,11 @@ if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then if test "$with_netcf" = "yes" ; then AC_DEFINE_UNQUOTED([WITH_NETCF], 1, [whether libnetcf is available to configure physical host network interfaces]) + AC_CHECK_LIB([netcf], [ncf_change_begin], [netcf_transactions=1], [netcf_transactions=0]) + if test "$netcf_transactions" = "1" ; then + AC_DEFINE_UNQUOTED([HAVE_NETCF_TRANSACTIONS], ["1"], + [we have sufficiently new version of netcf for transaction network API]) + fi fi fi AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"]) diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c index d5b401a..c94cbd5 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/netcf_driver.c @@ -540,6 +540,77 @@ cleanup: return ret; }

+#ifdef HAVE_NETCF_TRANSACTIONS +static int interfaceChangeBegin(virConnectPtr conn ATTRIBUTE_UNUSED, + unsigned int flags ATTRIBUTE_UNUSED) Both conn and flags *are* used in this method, and the 2 following methods.

Oops. yeah, fixed.

...

+ interfaceDriverLock(driver); + + ret = ncf_change_begin(driver->netcf, 0); + if (ret< 0) { + const char *errmsg, *details; + int errcode = ncf_error(driver->netcf,&errmsg,&details); + interfaceReportError(netcf_to_vir_err(errcode), + _("failed to begin transaction (netcf: %s - %s)"), + errmsg, details ? details : ""); I don't think including the word 'netcf' in the error message here is relevant. If 'details' is "", then this also adds a trailing ' - ' to

+ the error string. eg it needs to be

interfaceReportError(netcf_to_vir_err(errcode), _("failed to begin transaction: %s%s%s)"), errmsg, details ? " - " : "", details ? details : "");

Likewise in the other methods

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..

-----Original Message----- From: libvir-list-bounces@redhat.com [mailto:libvir-list- bounces@redhat.com] On Behalf Of Laine Stump Sent: Thursday, May 19, 2011 1:51 PM To: libvir-list@redhat.com Subject: [libvirt] [PATCHv2 7/7] interface: implement a test driver fornetwork config transaction API.

From: Michal Privoznik <mprivozn@redhat.com>

--- src/conf/interface_conf.c | 45 ++++++++++++++++++++++++ src/conf/interface_conf.h | 4 ++ src/test/test_driver.c | 83 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index f3848bd..4ff68aa 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -1227,6 +1227,51 @@ void virInterfaceObjListFree(virInterfaceObjListPtr interfaces) interfaces->count = 0; }

+int virInterfaceObjListClone(virConnectPtr conn, + virInterfaceObjListPtr src, + virInterfaceObjListPtr dest) +{ + int ret = -1; + unsigned int i, cnt; + + if (!src || !dest) + goto end; + + cnt = src->count; + for (i = 0; i < cnt; i++) { + virInterfaceDefPtr def = src->objs[i]->def; + virInterfaceDefPtr backup; + virInterfaceObjPtr iface; + char *xml = virInterfaceDefFormat(def); + + if (!xml) { + virReportOOMError(); + goto no_memory; + } + + if ((backup = virInterfaceDefParseString(xml)) == NULL) { + VIR_FREE(xml); + goto no_memory; + } + + VIR_FREE(xml); + if ((iface = virInterfaceAssignDef(dest, backup)) == NULL) + goto no_memory; + + virInterfaceObjUnlock(iface); + + conn->refs++; + } + + ret = cnt; +end: + return ret; + +no_memory: + virInterfaceObjListFree(dest); + goto end; +} + virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces, const virInterfaceDefPtr def) { diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index 6073b49..870a8ee 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -192,6 +192,10 @@ virInterfaceObjPtr virInterfaceFindByName(const virInterfaceObjListPtr void virInterfaceDefFree(virInterfaceDefPtr def); void virInterfaceObjFree(virInterfaceObjPtr iface); void virInterfaceObjListFree(virInterfaceObjListPtr vms); +int virInterfaceObjListClone(virConnectPtr conn, + virInterfaceObjListPtr src, + virInterfaceObjListPtr dest); +

virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces, const virInterfaceDefPtr def); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e86bc4e..b58c5d2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -85,6 +85,8 @@ struct _testConn { virDomainObjList domains; virNetworkObjList networks; virInterfaceObjList ifaces; + bool transaction_running; + virInterfaceObjList backupIfaces; virStoragePoolObjList pools; virNodeDeviceObjList devs; int numCells; @@ -3455,6 +3457,84 @@ cleanup: return ret; }

+static int testInterfaceChangeBegin(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn);

+ if (privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("there is another transaction " + "running.")); + goto cleanup; + } + + privconn->transaction_running = true;

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

+ if (virInterfaceObjListClone(conn, &privconn->ifaces, + &privconn->backupIfaces) < 0) + goto cleanup; + + ret = 0; +cleanup: + testDriverUnlock(privconn); + return ret; +} + +static int testInterfaceChangeCommit(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + + if (!privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("no transaction running, " + "nothing to be commited.")); + goto cleanup; + } + + virInterfaceObjListFree(&privconn->backupIfaces); + privconn->transaction_running = false; + + ret = 0; + +cleanup: + testDriverUnlock(privconn); + + return ret; +} + +static int testInterfaceChangeRollback(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + + if (!privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("no transaction running, " + "nothing to rollback.")); + goto cleanup; + } + + virInterfaceObjListFree(&privconn->ifaces); + privconn->ifaces.count = privconn->backupIfaces.count; + privconn->ifaces.objs = privconn->backupIfaces.objs; + privconn->backupIfaces.count = 0; + privconn->backupIfaces.objs = NULL; + + privconn->transaction_running = false; + + ret = 0; + +cleanup: + testDriverUnlock(privconn); + return ret; +}

static char *testInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags ATTRIBUTE_UNUSED) @@ -5428,6 +5508,9 @@ static virInterfaceDriver testInterfaceDriver = { .interfaceCreate = testInterfaceCreate, /* 0.7.0 */ .interfaceDestroy = testInterfaceDestroy, /* 0.7.0 */ .interfaceIsActive = testInterfaceIsActive, /* 0.7.3 */ + .interfaceChangeBegin = testInterfaceChangeBegin, /* 0.9.2 */ + .interfaceChangeCommit = testInterfaceChangeCommit, /* 0.9.2 */ + .interfaceChangeRollback = testInterfaceChangeRollback, /* 0.9.2 */ };

-- 1.7.3.4

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 05/24/2011 02:24 AM, Daniel Veillard wrote:

...

From: Michal Privoznik<mprivozn@redhat.com>

--- src/conf/interface_conf.c | 45 ++++++++++++++++++++++++ src/conf/interface_conf.h | 4 ++ src/test/test_driver.c | 83 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index f3848bd..4ff68aa 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -1227,6 +1227,51 @@ void virInterfaceObjListFree(virInterfaceObjListPtr interfaces) interfaces->count = 0; }

+int virInterfaceObjListClone(virConnectPtr conn, + virInterfaceObjListPtr src, + virInterfaceObjListPtr dest) +{ + int ret = -1; + unsigned int i, cnt; + + if (!src || !dest) + goto end; + + cnt = src->count; + for (i = 0; i< cnt; i++) { + virInterfaceDefPtr def = src->objs[i]->def; + virInterfaceDefPtr backup; + virInterfaceObjPtr iface; + char *xml = virInterfaceDefFormat(def); + + if (!xml) { + virReportOOMError(); + goto no_memory; + } + + if ((backup = virInterfaceDefParseString(xml)) == NULL) { + VIR_FREE(xml); + goto no_memory; + } + + VIR_FREE(xml); + if ((iface = virInterfaceAssignDef(dest, backup)) == NULL) + goto no_memory; + + virInterfaceObjUnlock(iface); + + conn->refs++; + } + + ret = cnt; +end: + return ret; + +no_memory: + virInterfaceObjListFree(dest); I'ts always a problem when the callee frees up memory allocated by

On Thu, May 19, 2011 at 04:51:29PM -0400, Laine Stump wrote: the caller in case of error, as it's so easy to mess things up. I would rather not do that here, and have the caller do the free if we fail. example: src == NULL, dest != NULL, and we leak dest.

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?

...

+ goto end; +} + virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces, const virInterfaceDefPtr def) { diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index 6073b49..870a8ee 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -192,6 +192,10 @@ virInterfaceObjPtr virInterfaceFindByName(const virInterfaceObjListPtr void virInterfaceDefFree(virInterfaceDefPtr def); void virInterfaceObjFree(virInterfaceObjPtr iface); void virInterfaceObjListFree(virInterfaceObjListPtr vms); +int virInterfaceObjListClone(virConnectPtr conn, + virInterfaceObjListPtr src, + virInterfaceObjListPtr dest); +

virInterfaceObjPtr virInterfaceAssignDef(virInterfaceObjListPtr interfaces, const virInterfaceDefPtr def); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e86bc4e..b58c5d2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -85,6 +85,8 @@ struct _testConn { virDomainObjList domains; virNetworkObjList networks; virInterfaceObjList ifaces; + bool transaction_running; + virInterfaceObjList backupIfaces; virStoragePoolObjList pools; virNodeDeviceObjList devs; int numCells; @@ -3455,6 +3457,84 @@ cleanup: return ret; }

+static int testInterfaceChangeBegin(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + if (privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("there is another transaction " + "running.")); + goto cleanup; + } + + privconn->transaction_running = true; + + if (virInterfaceObjListClone(conn,&privconn->ifaces, +&privconn->backupIfaces)< 0) + goto cleanup; + + ret = 0; +cleanup: + testDriverUnlock(privconn); + return ret; +} + +static int testInterfaceChangeCommit(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + + if (!privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("no transaction running, " + "nothing to be commited.")); + goto cleanup; + } + + virInterfaceObjListFree(&privconn->backupIfaces); + privconn->transaction_running = false; + + ret = 0; + +cleanup: + testDriverUnlock(privconn); + + return ret; +} + +static int testInterfaceChangeRollback(virConnectPtr conn, + unsigned int flags ATTRIBUTE_UNUSED) +{ + testConnPtr privconn = conn->privateData; + int ret = -1; + + testDriverLock(privconn); + + if (!privconn->transaction_running) { + testError(VIR_ERR_OPERATION_INVALID, _("no transaction running, " + "nothing to rollback.")); + goto cleanup; + } + + virInterfaceObjListFree(&privconn->ifaces); + privconn->ifaces.count = privconn->backupIfaces.count; + privconn->ifaces.objs = privconn->backupIfaces.objs; + privconn->backupIfaces.count = 0; + privconn->backupIfaces.objs = NULL; + + privconn->transaction_running = false; + + ret = 0; + +cleanup: + testDriverUnlock(privconn); + return ret; +}

static char *testInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags ATTRIBUTE_UNUSED) @@ -5428,6 +5508,9 @@ static virInterfaceDriver testInterfaceDriver = { .interfaceCreate = testInterfaceCreate, /* 0.7.0 */ .interfaceDestroy = testInterfaceDestroy, /* 0.7.0 */ .interfaceIsActive = testInterfaceIsActive, /* 0.7.3 */ + .interfaceChangeBegin = testInterfaceChangeBegin, /* 0.9.2 */ + .interfaceChangeCommit = testInterfaceChangeCommit, /* 0.9.2 */ + .interfaceChangeRollback = testInterfaceChangeRollback, /* 0.9.2 */ }; Except for the small nit on how virInterfaceObjListClone() handle errors, fine, and ACK once fixed (just do the free in the caller on error).

Daniel