Re: [libvirt] [PATCH 2/2] interface: Take interface status into account when starting and destroying
On 12/11/2013 11:16 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=956994
Currently, it is possible to start an interface that is already running:
# virsh iface-start eth2
Interface eth2 started
# echo $?
0
# virsh iface-start eth2
Interface eth2 started
# echo $?
0
# virsh iface-start eth2
Interface eth2 started
# echo $?
0
Same applies for destroying a dead interface. We should not allow such
state transitions.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/interface/interface_backend_netcf.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/interface/interface_backend_netcf.c
b/src/interface/interface_backend_netcf.c
index 2e681ec..c525ca9 100644
--- a/src/interface/interface_backend_netcf.c
+++ b/src/interface/interface_backend_netcf.c
@@ -944,6 +944,7 @@ static int netcfInterfaceCreate(virInterfacePtr ifinfo,
struct netcf_if *iface = NULL;
virInterfaceDefPtr def = NULL;
int ret = -1;
+ bool active;
virCheckFlags(0, -1);
@@ -962,6 +963,15 @@ static int netcfInterfaceCreate(virInterfacePtr ifinfo,
if (virInterfaceCreateEnsureACL(ifinfo->conn, def) < 0)
goto cleanup;
+ if (netcfInterfaceObjIsActive(iface, &active) < 0)
+ goto cleanup;
+
+ if (active) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("interface is already running"));
Do we want to use the word "running" here? The flags to
virConnectListAllInterfaces() use the term "active" rather than
"running". virConnectListAllNetworks() does likewise, and its error
message when trying to start an active network is "network is already
active".
On the other hand, the flags for virConnectListAllDomains() also uses
"active" in the flag name, but "domain is already running" for the
error
message.
So there is precedence for either, but I like "active" better (and an
interface is closer to a network than a domain :-)
+ goto cleanup;
+ }
+
ret = ncf_if_up(iface);
if (ret < 0) {
const char *errmsg, *details;
@@ -987,6 +997,7 @@ static int netcfInterfaceDestroy(virInterfacePtr ifinfo,
struct netcf_if *iface = NULL;
virInterfaceDefPtr def = NULL;
int ret = -1;
+ bool active;
virCheckFlags(0, -1);
@@ -1005,6 +1016,15 @@ static int netcfInterfaceDestroy(virInterfacePtr ifinfo,
if (virInterfaceDestroyEnsureACL(ifinfo->conn, def) < 0)
goto cleanup;
+ if (netcfInterfaceObjIsActive(iface, &active) < 0)
+ goto cleanup;
+
+ if (!active) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("interface is not running"));
Same comment here.
+ goto cleanup;
+ }
+
ret = ncf_if_down(iface);
if (ret < 0) {
const char *errmsg, *details;
ACK with the changes to the error messages (or without them if you have
a strong opinion in the other direction).
P.S. Be prepared for any fallout from the change in semantics!