3:16 a.m.
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
3:16 a.m.
New subject: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
This function barely wraps ncf_if_status() and error handling code.
Signed-off-by: Michal Privoznik <mprivozn(a)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
6:19 a.m.
New subject: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
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(a)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 :|
6:41 a.m.
New subject: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
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(a)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
1:03 p.m.
New subject: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
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
8:59 a.m.
New subject: [libvirt] [PATCH 1/2] interface: Introduce netcfInterfaceObjIsActive
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(a)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.
3:16 a.m.
New subject: [libvirt] [PATCH 2/2] interface: Take interface status into account when starting and destroying
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"));
+ 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
9:11 a.m.
New subject: [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!
4:23 a.m.
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.
8:22 a.m.
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
8:41 a.m.
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 :|
8:54 a.m.
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
8:57 a.m.
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 :|
8:32 a.m.
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
8:56 a.m.
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.
3994
days inactive
3999
days old
14 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Michal Privoznik