8:58 a.m.
Right now it is possible to undefine an active interface, or
destroy inactive. This patch add some checking to these operations
to prevent this. Also fix test driver.
---
src/interface/netcf_driver.c | 83 ++++++++++++++++++++++++++++++++++++------
src/test/test_driver.c | 5 +++
2 files changed, 76 insertions(+), 12 deletions(-)
diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c
index 855b5a3..bf590f3 100644
--- a/src/interface/netcf_driver.c
+++ b/src/interface/netcf_driver.c
@@ -119,6 +119,40 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf,
virInterfac
return iface;
}
+/*
+ * interfaceObjIsActive:
+ * @iface - interface
+ *
+ * Caller MUST have driver locked to prevent race condition.
+ * Returns:
+ * -1 on error
+ * 0 on inactive interface
+ * 1 on active interface
+ */
+static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+interfaceObjIsActive(struct interface_driver *driver,
+ struct netcf_if *iface,
+ virInterfacePtr ifinfo)
+{
+ int ret = -1;
+ unsigned int flags = 0;
+
+ if (ncf_if_status(iface, &flags) < 0) {
+ const char *errmsg, *details;
+ int errcode = ncf_error(driver->netcf, &errmsg, &details);
+ interfaceReportError(netcf_to_vir_err(errcode),
+ _("failed to get status of interface %s:
%s%s%s"),
+ ifinfo->name, errmsg, details ? " - " :
"",
+ details ? details : "");
+ goto cleanup;
+ }
+
+ ret = flags & NETCF_IFACE_ACTIVE ? 1 : 0;
+
+cleanup:
+ return ret;
+}
+
static virDrvOpenStatus interfaceOpenInterface(virConnectPtr conn,
virConnectAuthPtr auth ATTRIBUTE_UNUSED,
unsigned int flags)
@@ -438,6 +472,7 @@ static int interfaceUndefine(virInterfacePtr ifinfo) {
struct interface_driver *driver = ifinfo->conn->interfacePrivateData;
struct netcf_if *iface = NULL;
int ret = -1;
+ int active;
interfaceDriverLock(driver);
@@ -447,6 +482,17 @@ static int interfaceUndefine(virInterfacePtr ifinfo) {
goto cleanup;
}
+ active = interfaceObjIsActive(driver, iface, ifinfo);
+ if (active < 0) {
+ /* helper already reported error */
+ goto cleanup;
+ } else if (active) {
+ interfaceReportError(VIR_ERR_OPERATION_INVALID,
+ _("interface %s is active"),
+ ifinfo->name);
+ goto cleanup;
+ }
+
ret = ncf_if_undefine(iface);
if (ret < 0) {
const char *errmsg, *details;
@@ -470,6 +516,7 @@ static int interfaceCreate(virInterfacePtr ifinfo,
struct interface_driver *driver = ifinfo->conn->interfacePrivateData;
struct netcf_if *iface = NULL;
int ret = -1;
+ int active;
virCheckFlags(0, -1);
@@ -481,6 +528,17 @@ static int interfaceCreate(virInterfacePtr ifinfo,
goto cleanup;
}
+ active = interfaceObjIsActive(driver, iface, ifinfo);
+ if (active < 0) {
+ /* helper already reported error */
+ goto cleanup;
+ } else if (active) {
+ interfaceReportError(VIR_ERR_OPERATION_INVALID,
+ _("interface %s is already active"),
+ ifinfo->name);
+ goto cleanup;
+ }
+
ret = ncf_if_up(iface);
if (ret < 0) {
const char *errmsg, *details;
@@ -504,6 +562,7 @@ static int interfaceDestroy(virInterfacePtr ifinfo,
struct interface_driver *driver = ifinfo->conn->interfacePrivateData;
struct netcf_if *iface = NULL;
int ret = -1;
+ int active;
virCheckFlags(0, -1);
@@ -515,6 +574,17 @@ static int interfaceDestroy(virInterfacePtr ifinfo,
goto cleanup;
}
+ active = interfaceObjIsActive(driver, iface, ifinfo);
+ if (active < 0) {
+ /* helper already reported error */
+ goto cleanup;
+ } else if (!active) {
+ interfaceReportError(VIR_ERR_OPERATION_INVALID,
+ _("interface %s is not active"),
+ ifinfo->name);
+ goto cleanup;
+ }
+
ret = ncf_if_down(iface);
if (ret < 0) {
const char *errmsg, *details;
@@ -536,7 +606,6 @@ static int interfaceIsActive(virInterfacePtr ifinfo)
{
struct interface_driver *driver = ifinfo->conn->interfacePrivateData;
struct netcf_if *iface = NULL;
- unsigned int flags = 0;
int ret = -1;
interfaceDriverLock(driver);
@@ -547,17 +616,7 @@ static int interfaceIsActive(virInterfacePtr ifinfo)
goto cleanup;
}
- if (ncf_if_status(iface, &flags) < 0) {
- const char *errmsg, *details;
- int errcode = ncf_error(driver->netcf, &errmsg, &details);
- interfaceReportError(netcf_to_vir_err(errcode),
- _("failed to get status of interface %s:
%s%s%s"),
- ifinfo->name, errmsg, details ? " - " :
"",
- details ? details : "");
- goto cleanup;
- }
-
- ret = flags & NETCF_IFACE_ACTIVE ? 1 : 0;
+ ret = interfaceObjIsActive(driver, iface, ifinfo);
cleanup:
ncf_if_free(iface);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index f3fb320..d165586 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3648,6 +3648,11 @@ static int testInterfaceUndefine(virInterfacePtr iface)
goto cleanup;
}
+ if (privinterface->active != 0) {
+ testError(VIR_ERR_OPERATION_INVALID, NULL);
+ goto cleanup;
+ }
+
virInterfaceRemove(&privconn->ifaces,
privinterface);
ret = 0;
--
1.7.5.rc3
9:29 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 07/15/2011 07:58 AM, Michal Privoznik wrote:
Right now it is possible to undefine an active interface, or
destroy inactive. This patch add some checking to these operations
to prevent this. Also fix test driver.
I'm inclined to NACK this on design principles (I haven't read the patch
itself, though). Given the discussion about domains and undefine, the
ability to undefine an active interface is a feature, provided we
support the concept of a transient interface like we do for transient
domains.
That is, we have the following transitions:
nothing -> transient/running via Create
nothing -> persistent/inactive via Define
persistent/inactive -> persistent/active via Start
persistent/inactive -> gone via Undefine
persistent/running -> persistent/inactive via Destroy
persistent/running -> transient/running via Undefine
transient/running -> gone via Destroy
transient/running -> persistent/running via Define
and rejecting Undefine on a running interface would prevent the ability
to transistion a persistent over to a transient interface.
On the other hand, if we don't support transient interfaces, then the
above analysis which works for domains would have to be adjusted for
interfaces, so you may have something to patch after all.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:36 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 15.07.2011 16:29, Eric Blake wrote:
On 07/15/2011 07:58 AM, Michal Privoznik wrote:
> Right now it is possible to undefine an active interface, or
> destroy inactive. This patch add some checking to these operations
> to prevent this. Also fix test driver.
I'm inclined to NACK this on design principles (I haven't read the patch
itself, though). Given the discussion about domains and undefine, the
ability to undefine an active interface is a feature, provided we
support the concept of a transient interface like we do for transient
domains.
That is, we have the following transitions:
nothing -> transient/running via Create
nothing -> persistent/inactive via Define
persistent/inactive -> persistent/active via Start
persistent/inactive -> gone via Undefine
persistent/running -> persistent/inactive via Destroy
persistent/running -> transient/running via Undefine
transient/running -> gone via Destroy
transient/running -> persistent/running via Define
and rejecting Undefine on a running interface would prevent the ability
to transistion a persistent over to a transient interface.
On the other hand, if we don't support transient interfaces, then the
above analysis which works for domains would have to be adjusted for
interfaces, so you may have something to patch after all.
Well, although we have function interfaceCreate, it is actually (from
semantic POV) interfaceStart, because it just start inactive but defined
interface. So we do not support transient interfaces. Therefore
transitions for interfaces are slightly different from transitions for
domains. That's why I think we do need this patch.
Michal
9:45 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 07/15/2011 08:36 AM, Michal Privoznik wrote:
> On the other hand, if we don't support transient interfaces,
then the
> above analysis which works for domains would have to be adjusted for
> interfaces, so you may have something to patch after all.
>
Well, although we have function interfaceCreate, it is actually (from
semantic POV) interfaceStart, because it just start inactive but defined
interface. So we do not support transient interfaces. Therefore
transitions for interfaces are slightly different from transitions for
domains. That's why I think we do need this patch.
Let's nail down the transitions that we plan to support, then, just as I
did earlier for domains.
It would be even cooler to have a life cycle diagram with the API used
to transition between states documented somewhere. I seem to recall
seeing one for domains once, but couldn't find it in 5 minutes of
searching right now.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:48 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 15.07.2011 16:45, Eric Blake wrote:
On 07/15/2011 08:36 AM, Michal Privoznik wrote:
>> On the other hand, if we don't support transient interfaces, then the
>> above analysis which works for domains would have to be adjusted for
>> interfaces, so you may have something to patch after all.
>>
> Well, although we have function interfaceCreate, it is actually (from
> semantic POV) interfaceStart, because it just start inactive but defined
> interface. So we do not support transient interfaces. Therefore
> transitions for interfaces are slightly different from transitions for
> domains. That's why I think we do need this patch.
Let's nail down the transitions that we plan to support, then, just as I
did earlier for domains.
It would be even cooler to have a life cycle diagram with the API used
to transition between states documented somewhere. I seem to recall
seeing one for domains once, but couldn't find it in 5 minutes of
searching right now.
You mean
http://wiki.libvirt.org/page/VM_lifecycle#States_that_a_guest_domain_can_...
?
9:54 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 07/15/2011 08:48 AM, Michal Privoznik wrote:
On 15.07.2011 16:45, Eric Blake wrote:
> On 07/15/2011 08:36 AM, Michal Privoznik wrote:
>>> On the other hand, if we don't support transient interfaces, then the
>>> above analysis which works for domains would have to be adjusted for
>>> interfaces, so you may have something to patch after all.
>>>
>> Well, although we have function interfaceCreate, it is actually (from
>> semantic POV) interfaceStart, because it just start inactive but defined
>> interface. So we do not support transient interfaces. Therefore
>> transitions for interfaces are slightly different from transitions for
>> domains. That's why I think we do need this patch.
>
> Let's nail down the transitions that we plan to support, then, just as I
> did earlier for domains.
>
> It would be even cooler to have a life cycle diagram with the API used
> to transition between states documented somewhere. I seem to recall
> seeing one for domains once, but couldn't find it in 5 minutes of
> searching right now.
>
You mean
http://wiki.libvirt.org/page/VM_lifecycle#States_that_a_guest_domain_can_...
You found a different one than me, but that one also has the problem of
no mention of a transient domain.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:02 a.m.
2011/7/15 Eric Blake <eblake(a)redhat.com>:
On 07/15/2011 08:48 AM, Michal Privoznik wrote:
> On 15.07.2011 16:45, Eric Blake wrote:
>> On 07/15/2011 08:36 AM, Michal Privoznik wrote:
>>>> On the other hand, if we don't support transient interfaces, then
the
>>>> above analysis which works for domains would have to be adjusted for
>>>> interfaces, so you may have something to patch after all.
>>>>
>>> Well, although we have function interfaceCreate, it is actually (from
>>> semantic POV) interfaceStart, because it just start inactive but defined
>>> interface. So we do not support transient interfaces. Therefore
>>> transitions for interfaces are slightly different from transitions for
>>> domains. That's why I think we do need this patch.
>>
>> Let's nail down the transitions that we plan to support, then, just as I
>> did earlier for domains.
>>
>> It would be even cooler to have a life cycle diagram with the API used
>> to transition between states documented somewhere. I seem to recall
>> seeing one for domains once, but couldn't find it in 5 minutes of
>> searching right now.
>>
> You mean
> http://wiki.libvirt.org/page/VM_lifecycle#States_that_a_guest_domain_can_...
You found a different one than me, but that one also has the problem of
no mention of a transient domain.
That one actually covers transient domain partly. Look at the
create/shutdown transition between undefined and running states.
--
Matthias Bolte
http://photron.blogspot.com
9:49 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 07/15/2011 08:45 AM, Eric Blake wrote:
On 07/15/2011 08:36 AM, Michal Privoznik wrote:
>> On the other hand, if we don't support transient interfaces, then the
>> above analysis which works for domains would have to be adjusted for
>> interfaces, so you may have something to patch after all.
>>
> Well, although we have function interfaceCreate, it is actually (from
> semantic POV) interfaceStart, because it just start inactive but defined
> interface. So we do not support transient interfaces. Therefore
> transitions for interfaces are slightly different from transitions for
> domains. That's why I think we do need this patch.
Let's nail down the transitions that we plan to support, then, just as I
did earlier for domains.
It would be even cooler to have a life cycle diagram with the API used
to transition between states documented somewhere. I seem to recall
seeing one for domains once, but couldn't find it in 5 minutes of
searching right now.
Found it:
http://libvirt.org/guide/html-single/#Application_Development_Guide-Guest...
That diagram completely lacks transient domains. It also shows
persistent/running to persistent/inactive via Shutdown, while I
mentioned Destroy (both APIs work for that transition, although Shutdown
requires guest response).
Something that could certainly use some TLC!
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10 a.m.
2011/7/15 Eric Blake <eblake(a)redhat.com>:
On 07/15/2011 08:45 AM, Eric Blake wrote:
> On 07/15/2011 08:36 AM, Michal Privoznik wrote:
>>> On the other hand, if we don't support transient interfaces, then the
>>> above analysis which works for domains would have to be adjusted for
>>> interfaces, so you may have something to patch after all.
>>>
>> Well, although we have function interfaceCreate, it is actually (from
>> semantic POV) interfaceStart, because it just start inactive but defined
>> interface. So we do not support transient interfaces. Therefore
>> transitions for interfaces are slightly different from transitions for
>> domains. That's why I think we do need this patch.
>
> Let's nail down the transitions that we plan to support, then, just as I
> did earlier for domains.
>
> It would be even cooler to have a life cycle diagram with the API used
> to transition between states documented somewhere. I seem to recall
> seeing one for domains once, but couldn't find it in 5 minutes of
> searching right now.
Found it:
http://libvirt.org/guide/html-single/#Application_Development_Guide-Guest...
For some reason the anchor doesn't work for me, just for reference a
link that involves less scrolling :)
http://libvirt.org/guide/html/Application_Development_Guide-Guest_Domains...
That diagram completely lacks transient domains. It also shows
persistent/running to persistent/inactive via Shutdown, while I
mentioned Destroy (both APIs work for that transition, although Shutdown
requires guest response).
Something that could certainly use some TLC!
Also that diagram is wrong about the saved state. There is nothing
like that in context of virDomainSave/virDomainRestore. Also the
diagram misses managedsave. In the context of managedsave there is
actually a saved state.
--
Matthias Bolte
http://photron.blogspot.com
12:21 p.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 07/15/2011 10:36 AM, Michal Privoznik wrote:
On 15.07.2011 16:29, Eric Blake wrote:
> On 07/15/2011 07:58 AM, Michal Privoznik wrote:
>> Right now it is possible to undefine an active interface, or
>> destroy inactive. This patch add some checking to these operations
>> to prevent this. Also fix test driver.
>
> I'm inclined to NACK this on design principles (I haven't read the patch
> itself, though). Given the discussion about domains and undefine, the
> ability to undefine an active interface is a feature, provided we
> support the concept of a transient interface like we do for transient
> domains.
>
> That is, we have the following transitions:
>
> nothing -> transient/running via Create
> nothing -> persistent/inactive via Define
>
> persistent/inactive -> persistent/active via Start
> persistent/inactive -> gone via Undefine
>
> persistent/running -> persistent/inactive via Destroy
> persistent/running -> transient/running via Undefine
>
> transient/running -> gone via Destroy
> transient/running -> persistent/running via Define
>
> and rejecting Undefine on a running interface would prevent the ability
> to transistion a persistent over to a transient interface.
>
> On the other hand, if we don't support transient interfaces, then the
> above analysis which works for domains would have to be adjusted for
> interfaces, so you may have something to patch after all.
>
Well, although we have function interfaceCreate, it is actually (from
semantic POV) interfaceStart, because it just start inactive but
defined interface. So we do not support transient interfaces.
Therefore transitions for interfaces are slightly different from
transitions for domains. That's why I think we do need this patch.
Since I was the original author of this file, I guess I'd better get
into the conversation :-)
The odd part of this is that I recall having a conversation about
allowing/not allowing undefine of an interface that is active, and I
*thought* that it didn't allow it (but obviously it does).
A couple of points:
1) The fact that we don't support transient interfaces now doesn't
preclude supporting them in the future (although we may use some method
other than netcf to do it).
2) We should be careful changing this, in case it has any effects on
virt-manager's use of the API.
2:02 a.m.
New subject: [libvirt] [PATCH] interface: Check for interface (in-)activity on some operations
On 18.07.2011 19:21, Laine Stump wrote:
On 07/15/2011 10:36 AM, Michal Privoznik wrote:
> On 15.07.2011 16:29, Eric Blake wrote:
>> On 07/15/2011 07:58 AM, Michal Privoznik wrote:
>>> Right now it is possible to undefine an active interface, or
>>> destroy inactive. This patch add some checking to these operations
>>> to prevent this. Also fix test driver.
>>
>> I'm inclined to NACK this on design principles (I haven't read the patch
>> itself, though). Given the discussion about domains and undefine, the
>> ability to undefine an active interface is a feature, provided we
>> support the concept of a transient interface like we do for transient
>> domains.
>>
>> That is, we have the following transitions:
>>
>> nothing -> transient/running via Create
>> nothing -> persistent/inactive via Define
>>
>> persistent/inactive -> persistent/active via Start
>> persistent/inactive -> gone via Undefine
>>
>> persistent/running -> persistent/inactive via Destroy
>> persistent/running -> transient/running via Undefine
>>
>> transient/running -> gone via Destroy
>> transient/running -> persistent/running via Define
>>
>> and rejecting Undefine on a running interface would prevent the ability
>> to transistion a persistent over to a transient interface.
>>
>> On the other hand, if we don't support transient interfaces, then the
>> above analysis which works for domains would have to be adjusted for
>> interfaces, so you may have something to patch after all.
>>
> Well, although we have function interfaceCreate, it is actually (from
> semantic POV) interfaceStart, because it just start inactive but
> defined interface. So we do not support transient interfaces.
> Therefore transitions for interfaces are slightly different from
> transitions for domains. That's why I think we do need this patch.
Since I was the original author of this file, I guess I'd better get
into the conversation :-)
The odd part of this is that I recall having a conversation about
allowing/not allowing undefine of an interface that is active, and I
*thought* that it didn't allow it (but obviously it does).
A couple of points:
1) The fact that we don't support transient interfaces now doesn't
preclude supporting them in the future (although we may use some method
other than netcf to do it).
Agree. So what about - letting this in, and once we decide to support
transient interfaces, we can (can't we?) remove this. I think it is a
bug to allow interface go transient as we don't support them now.
2) We should be careful changing this, in case it has any effects on
virt-manager's use of the API.
Agree in being careful. But as I've said, it is from my POV a bug which
in this case might expose bug in there.
Michal
4875
days inactive
4879
days old
10 comments
4 participants
participants (4)
-
Eric Blake -
Laine Stump -
Matthias Bolte -
Michal Privoznik