[libvirt] [PATCH 0/2] Fix interface state transitions logic
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver. Michal Privoznik (2): interface: Introduce netcfInterfaceObjIsActive interface: Take interface status into account when starting and destroying src/interface/interface_backend_netcf.c | 77 +++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 24 deletions(-) -- 1.8.5.1
This function barely wraps ncf_if_status() and error handling code. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/interface/interface_backend_netcf.c | 57 +++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index c4e18c4..2e681ec 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac return iface; } +static int +netcfInterfaceObjIsActive(struct netcf_if *iface, + bool *active) +{ + int ret = -1; + unsigned int flags = 0; + + virObjectRef(driverState); + if (ncf_if_status(iface, &flags) < 0) { + const char *errmsg, *details; + int errcode = ncf_error(driverState->netcf, &errmsg, &details); + virReportError(netcf_to_vir_err(errcode), + _("failed to get status of interface %s: %s%s%s"), + ncf_if_name(iface), errmsg, details ? " - " : "", + details ? details : ""); + goto cleanup; + } + + *active = flags & NETCF_IFACE_ACTIVE; + ret = 0; + +cleanup: + virObjectUnref(driverState); + return ret; +} + static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -539,7 +565,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn, struct netcf_if *iface = NULL; virInterfacePtr *tmp_iface_objs = NULL; virInterfacePtr iface_obj = NULL; - unsigned int status; + bool active; int niface_objs = 0; int ret = -1; char **names = NULL; @@ -611,15 +637,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn, } } - if (ncf_if_status(iface, &status) < 0) { - const char *errmsg, *details; - int errcode = ncf_error(driver->netcf, &errmsg, &details); - virReportError(netcf_to_vir_err(errcode), - _("failed to get status of interface %s: %s%s%s"), - names[i], errmsg, details ? " - " : "", - details ? details : ""); + if (netcfInterfaceObjIsActive(iface, &active) < 0) goto cleanup; - } if (!(def = netcfGetMinimalDefForDevice(iface))) goto cleanup; @@ -636,10 +655,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn, * except active|inactive are supported. */ if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) && - !((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && - (status & NETCF_IFACE_ACTIVE)) || - (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && - (status & NETCF_IFACE_INACTIVE)))) { + !((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && active) || + (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && !active))) { ncf_if_free(iface); iface = NULL; continue; @@ -1010,9 +1027,9 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo) { virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; - unsigned int flags = 0; virInterfaceDefPtr def = NULL; int ret = -1; + bool active; virObjectLock(driver); @@ -1022,24 +1039,16 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo) goto cleanup; } - if (!(def = netcfGetMinimalDefForDevice(iface))) goto cleanup; if (virInterfaceIsActiveEnsureACL(ifinfo->conn, def) < 0) goto cleanup; - if (ncf_if_status(iface, &flags) < 0) { - const char *errmsg, *details; - int errcode = ncf_error(driver->netcf, &errmsg, &details); - virReportError(netcf_to_vir_err(errcode), - _("failed to get status of interface %s: %s%s%s"), - ifinfo->name, errmsg, details ? " - " : "", - details ? details : ""); + if (netcfInterfaceObjIsActive(iface, &active) < 0) goto cleanup; - } - ret = flags & NETCF_IFACE_ACTIVE ? 1 : 0; + ret = active ? 1 : 0; cleanup: ncf_if_free(iface); -- 1.8.5.1
On Wed, Dec 11, 2013 at 10:16:37AM +0100, Michal Privoznik wrote:
This function barely wraps ncf_if_status() and error handling code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/interface/interface_backend_netcf.c | 57 +++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index c4e18c4..2e681ec 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac return iface; }
+static int +netcfInterfaceObjIsActive(struct netcf_if *iface, + bool *active) +{ + int ret = -1; + unsigned int flags = 0; + + virObjectRef(driverState);
What's the ref / unref of driverState for ? The code you're replacing doesn't do that, and AFAICT this shouldn't be required since we're always calling this from public API context
+ if (ncf_if_status(iface, &flags) < 0) { + const char *errmsg, *details; + int errcode = ncf_error(driverState->netcf, &errmsg, &details); + virReportError(netcf_to_vir_err(errcode), + _("failed to get status of interface %s: %s%s%s"), + ncf_if_name(iface), errmsg, details ? " - " : "", + details ? details : ""); + goto cleanup; + } + + *active = flags & NETCF_IFACE_ACTIVE; + ret = 0; + +cleanup: + virObjectUnref(driverState); + return ret; +} +
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 11.12.2013 13:19, Daniel P. Berrange wrote:
On Wed, Dec 11, 2013 at 10:16:37AM +0100, Michal Privoznik wrote:
This function barely wraps ncf_if_status() and error handling code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/interface/interface_backend_netcf.c | 57 +++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index c4e18c4..2e681ec 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac return iface; }
+static int +netcfInterfaceObjIsActive(struct netcf_if *iface, + bool *active) +{ + int ret = -1; + unsigned int flags = 0; + + virObjectRef(driverState);
What's the ref / unref of driverState for ? The code you're replacing doesn't do that, and AFAICT this shouldn't be required since we're always calling this from public API context
Well, for now it is not required, true. But I was thinking if somebody rewrites something this part wouldn't need any change as it's already foolproof. But I don't mind dropping ref and unref. Michal
On 12/11/2013 05:41 AM, Michal Privoznik wrote:
+static int +netcfInterfaceObjIsActive(struct netcf_if *iface, + bool *active) +{ + int ret = -1; + unsigned int flags = 0; + + virObjectRef(driverState);
What's the ref / unref of driverState for ? The code you're replacing doesn't do that, and AFAICT this shouldn't be required since we're always calling this from public API context
Well, for now it is not required, true. But I was thinking if somebody rewrites something this part wouldn't need any change as it's already foolproof. But I don't mind dropping ref and unref.
Spurious locking now for future code patches is counter-productive; if someone adds code that needs locking, we can add the locking at that point (and ideally keeping things lockless is the better approach). So yes, let's drop the ref and unref. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/11/2013 11:16 AM, Michal Privoznik wrote:
This function barely wraps ncf_if_status() and error handling code.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/interface/interface_backend_netcf.c | 57 +++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index c4e18c4..2e681ec 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -238,6 +238,32 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac return iface; }
+static int +netcfInterfaceObjIsActive(struct netcf_if *iface, + bool *active) +{ + int ret = -1; + unsigned int flags = 0; + + virObjectRef(driverState); + if (ncf_if_status(iface, &flags) < 0) { + const char *errmsg, *details; + int errcode = ncf_error(driverState->netcf, &errmsg, &details); + virReportError(netcf_to_vir_err(errcode), + _("failed to get status of interface %s: %s%s%s"), + ncf_if_name(iface), errmsg, details ? " - " : "", + details ? details : ""); + goto cleanup; + } + + *active = flags & NETCF_IFACE_ACTIVE; + ret = 0; + +cleanup: + virObjectUnref(driverState); + return ret; +} + static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -539,7 +565,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn, struct netcf_if *iface = NULL; virInterfacePtr *tmp_iface_objs = NULL; virInterfacePtr iface_obj = NULL; - unsigned int status; + bool active; int niface_objs = 0; int ret = -1; char **names = NULL; @@ -611,15 +637,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn, } }
- if (ncf_if_status(iface, &status) < 0) { - const char *errmsg, *details; - int errcode = ncf_error(driver->netcf, &errmsg, &details); - virReportError(netcf_to_vir_err(errcode), - _("failed to get status of interface %s: %s%s%s"), - names[i], errmsg, details ? " - " : "", - details ? details : ""); + if (netcfInterfaceObjIsActive(iface, &active) < 0) goto cleanup; - }
if (!(def = netcfGetMinimalDefForDevice(iface))) goto cleanup; @@ -636,10 +655,8 @@ netcfConnectListAllInterfaces(virConnectPtr conn, * except active|inactive are supported. */ if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) && - !((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && - (status & NETCF_IFACE_ACTIVE)) || - (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && - (status & NETCF_IFACE_INACTIVE)))) { + !((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) && active) || + (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) && !active))) { ncf_if_free(iface); iface = NULL; continue; @@ -1010,9 +1027,9 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo) { virNetcfDriverStatePtr driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; - unsigned int flags = 0; virInterfaceDefPtr def = NULL; int ret = -1; + bool active;
virObjectLock(driver);
@@ -1022,24 +1039,16 @@ static int netcfInterfaceIsActive(virInterfacePtr ifinfo) goto cleanup; }
- if (!(def = netcfGetMinimalDefForDevice(iface))) goto cleanup;
if (virInterfaceIsActiveEnsureACL(ifinfo->conn, def) < 0) goto cleanup;
- if (ncf_if_status(iface, &flags) < 0) { - const char *errmsg, *details; - int errcode = ncf_error(driver->netcf, &errmsg, &details); - virReportError(netcf_to_vir_err(errcode), - _("failed to get status of interface %s: %s%s%s"), - ifinfo->name, errmsg, details ? " - " : "", - details ? details : ""); + if (netcfInterfaceObjIsActive(iface, &active) < 0) goto cleanup; - }
- ret = flags & NETCF_IFACE_ACTIVE ? 1 : 0; + ret = active ? 1 : 0;
cleanup: ncf_if_free(iface);
This looks fine other than the extra red/unref of the driver state object that Dan already mentioned. ACK with that removed.
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@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")); + 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")); + goto cleanup; + } + ret = ncf_if_down(iface); if (ret < 0) { const char *errmsg, *details; -- 1.8.5.1
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@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!
On 12/11/2013 11:16 AM, Michal Privoznik wrote:
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver.
I recall that someone brought up this issue before, and we didn't change the code. If I remember correctly, the reason was that the virInterfaceCreate() API is basically just a wrapper around /sbin/ifup, and /sbin/ifup can be called (and returns success after doing something useful - see below) if the interface is already up. Why would you want to do that? Well, calling ifup on an interface that is already up causes it to be "re-started", renewing its IP (and other) configuration from any changes that have been made, *without* needing to ifdown the interface in the process (and completely losing network connectivity, possibly making it impossible for the program calling the API to re-start the interface). (It's not directly applicable in this case, but when a bridge device is added, subsuming an existing ethernet, we rely on allowing virInterfaceCreate() to be called for the new bridge device even though the subordinate ethernet is already up; this permits us to add a bridge to the host's config without losing network connectivity.) I'm not convinced that it's a bad thing that virInterfaceCreate/Destroy can, by definition, be called when the device is already in the desired state, and wouldn't want to end up with other unforeseen problems just because we disabled it.
On 12.12.2013 11:23, Laine Stump wrote:
On 12/11/2013 11:16 AM, Michal Privoznik wrote:
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver.
I recall that someone brought up this issue before, and we didn't change the code. If I remember correctly, the reason was that the virInterfaceCreate() API is basically just a wrapper around /sbin/ifup, and /sbin/ifup can be called (and returns success after doing something useful - see below) if the interface is already up.
Why would you want to do that? Well, calling ifup on an interface that is already up causes it to be "re-started", renewing its IP (and other) configuration from any changes that have been made, *without* needing to ifdown the interface in the process (and completely losing network connectivity, possibly making it impossible for the program calling the API to re-start the interface). (It's not directly applicable in this case, but when a bridge device is added, subsuming an existing ethernet, we rely on allowing virInterfaceCreate() to be called for the new bridge device even though the subordinate ethernet is already up; this permits us to add a bridge to the host's config without losing network connectivity.)
I'm not convinced that it's a bad thing that virInterfaceCreate/Destroy can, by definition, be called when the device is already in the desired state, and wouldn't want to end up with other unforeseen problems just because we disabled it.
Well, by the same token virDomainCreate() called against a running domain should impersonate all the changes made to config, e.g. (un-)plug devices, change VNC/SPICE listen address, etc. But it's not doing that. Anyway, I can live with status quo. Michal
On Thu, Dec 12, 2013 at 12:23:48PM +0200, Laine Stump wrote:
On 12/11/2013 11:16 AM, Michal Privoznik wrote:
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver.
I recall that someone brought up this issue before, and we didn't change the code. If I remember correctly, the reason was that the virInterfaceCreate() API is basically just a wrapper around /sbin/ifup, and /sbin/ifup can be called (and returns success after doing something useful - see below) if the interface is already up.
Why would you want to do that? Well, calling ifup on an interface that is already up causes it to be "re-started", renewing its IP (and other) configuration from any changes that have been made, *without* needing to ifdown the interface in the process (and completely losing network connectivity, possibly making it impossible for the program calling the API to re-start the interface). (It's not directly applicable in this case, but when a bridge device is added, subsuming an existing ethernet, we rely on allowing virInterfaceCreate() to be called for the new bridge device even though the subordinate ethernet is already up; this permits us to add a bridge to the host's config without losing network connectivity.)
The usage you describe here is not within the scope of the virInterfaceCreate() API IMHO. If we want users to have the ability to "re start" an interface without taking it offline first, then we should add another API that explicitly supports that use case.
I'm not convinced that it's a bad thing that virInterfaceCreate/Destroy can, by definition, be called when the device is already in the desired state, and wouldn't want to end up with other unforeseen problems just because we disabled it.
Overloading a single virInterfaceCreate to support two different use cases puts applications in an impossible position if they *want* to see an error from attempting to start an already active interface. They can't just check virInterfaceIsActive before calling the virInterfaceCreate API because that is racy from the apps POV. If we explicitly have separate APIs for starting vs refreshing the config then we can cleanly support all use scenarios. 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 12/12/2013 07:41 AM, Daniel P. Berrange wrote:
The usage you describe here is not within the scope of the virInterfaceCreate() API IMHO. If we want users to have the ability to "re start" an interface without taking it offline first, then we should add another API that explicitly supports that use case.
Or even a flag to the existing argument that says to restart if already running, where the normal case of not having the flag is an error if already running.
Overloading a single virInterfaceCreate to support two different use cases puts applications in an impossible position if they *want* to see an error from attempting to start an already active interface.
Overloading without the use of a flag bit, that is. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Dec 12, 2013 at 07:54:34AM -0700, Eric Blake wrote:
On 12/12/2013 07:41 AM, Daniel P. Berrange wrote:
The usage you describe here is not within the scope of the virInterfaceCreate() API IMHO. If we want users to have the ability to "re start" an interface without taking it offline first, then we should add another API that explicitly supports that use case.
Or even a flag to the existing argument that says to restart if already running, where the normal case of not having the flag is an error if already running.
Overloading a single virInterfaceCreate to support two different use cases puts applications in an impossible position if they *want* to see an error from attempting to start an already active interface.
Overloading without the use of a flag bit, that is.
In this scenario I'd prefer a separate API. Flags are good when they are tweaking some aspect of behaviour but leaving the overall semantics of the method intact. A virInterfaceCreate API call is a lifecycle operation, which should result in a lifecycle change (and event emission) upon success. Just doing a refresh of the config isn't a lifecycle change and would not be expected to trigger any lifecycle events either. This is different enough that I think it would belong in a separate API. 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 11.12.2013 10:16, Michal Privoznik wrote:
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver.
Michal Privoznik (2): interface: Introduce netcfInterfaceObjIsActive interface: Take interface status into account when starting and destroying
src/interface/interface_backend_netcf.c | 77 +++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 24 deletions(-)
So it seems to me like we have agreement that the current state transitions should be prohibited. If that's the case, can somebody please review the patches? Thanks! Michal
On 12/16/2013 04:32 PM, Michal Privoznik wrote:
On 11.12.2013 10:16, Michal Privoznik wrote:
Right now it's possible to start an interface that is already running, or destroy an interface multiple times. Such state transitions are not allowed and we check for such cases explicitly in other areas like qemu driver.
Michal Privoznik (2): interface: Introduce netcfInterfaceObjIsActive interface: Take interface status into account when starting and destroying
src/interface/interface_backend_netcf.c | 77 +++++++++++++++++++++++---------- 1 file changed, 53 insertions(+), 24 deletions(-)
So it seems to me like we have agreement that the current state transitions should be prohibited.
So I guess absence of protest is considered agreement? :-) Yeah, I suppose I can live with it. Dan does have a point that the current situation (although it is reproducing the initscripts behavior) is overloading a single API with multiple behaviors.
If that's the case, can somebody please review the patches? Thanks!
I'll look through them now.
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Michal Privoznik