3:09 p.m.
Jiri Denemark (4):
xen: Make xenDaemon*DeviceFlags errors less confusing
xen: Fix logic bug in xenDaemon*DeviceFlags
xen: xenXMDomain*DeviceFlags should obey all flags
xen: Fix virDomain{At,De}tachDevice
src/xen/xen_driver.c | 24 ++++++++++++++++-----
src/xen/xend_internal.c | 51 ++++++++++++++++++++++++----------------------
src/xen/xm_internal.c | 14 +++++++++++-
3 files changed, 57 insertions(+), 32 deletions(-)
--
1.7.3.1
3:09 p.m.
New subject: [libvirt] [PATCH 1/4] xen: Make xenDaemon*DeviceFlags errors less confusing
When a user calls to virDomain{Attach,Detach,Update}DeviceFlags() with
flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE on an inactive guest running on
an old Xen hypervisor (such as RHEL-5) xend_internal driver reports:
Xend version does not support modifying persistent config
which is pretty confusing since no-one requested to modify persistent
config.
---
src/xen/xend_internal.c | 36 ++++++++++++++++++------------------
1 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index fce0233..1318bd4 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -3878,6 +3878,12 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml,
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
if (domain->id < 0) {
+ /* Cannot modify live config if domain is inactive */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ virXendError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Cannot modify live config if domain is
inactive"));
+ return -1;
+ }
/* If xendConfigVersion < 3 only live config can be changed */
if (priv->xendConfigVersion < 3) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -3885,12 +3891,6 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml,
"persistent config"));
return -1;
}
- /* Cannot modify live config if domain is inactive */
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
- virXendError(VIR_ERR_OPERATION_INVALID, "%s",
- _("Cannot modify live config if domain is
inactive"));
- return -1;
- }
} else {
/* Only live config can be changed if xendConfigVersion < 3 */
if (priv->xendConfigVersion < 3 &&
@@ -4017,6 +4017,12 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml,
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
if (domain->id < 0) {
+ /* Cannot modify live config if domain is inactive */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ virXendError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Cannot modify live config if domain is
inactive"));
+ return -1;
+ }
/* If xendConfigVersion < 3 only live config can be changed */
if (priv->xendConfigVersion < 3) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4024,12 +4030,6 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml,
"persistent config"));
return -1;
}
- /* Cannot modify live config if domain is inactive */
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
- virXendError(VIR_ERR_OPERATION_INVALID, "%s",
- _("Cannot modify live config if domain is
inactive"));
- return -1;
- }
} else {
/* Only live config can be changed if xendConfigVersion < 3 */
if (priv->xendConfigVersion < 3 &&
@@ -4128,6 +4128,12 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml,
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
if (domain->id < 0) {
+ /* Cannot modify live config if domain is inactive */
+ if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ virXendError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Cannot modify live config if domain is
inactive"));
+ return -1;
+ }
/* If xendConfigVersion < 3 only live config can be changed */
if (priv->xendConfigVersion < 3) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -4135,12 +4141,6 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml,
"persistent config"));
return -1;
}
- /* Cannot modify live config if domain is inactive */
- if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
- virXendError(VIR_ERR_OPERATION_INVALID, "%s",
- _("Cannot modify live config if domain is
inactive"));
- return -1;
- }
} else {
/* Only live config can be changed if xendConfigVersion < 3 */
if (priv->xendConfigVersion < 3 &&
--
1.7.3.1
4:07 p.m.
New subject: [libvirt] [PATCH 1/4] xen: Make xenDaemon*DeviceFlags errors less confusing
On 10/01/2010 02:09 PM, Jiri Denemark wrote:
When a user calls to virDomain{Attach,Detach,Update}DeviceFlags()
with
flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE on an inactive guest running on
an old Xen hypervisor (such as RHEL-5) xend_internal driver reports:
Xend version does not support modifying persistent config
which is pretty confusing since no-one requested to modify persistent
config.
Hmm - given the recent discussion on vcpus (which is probably what made
you look at this, right?)...
---
src/xen/xend_internal.c | 36 ++++++++++++++++++------------------
1 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index fce0233..1318bd4 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -3878,6 +3878,12 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml,
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
if (domain->id< 0) {
+ /* Cannot modify live config if domain is inactive */
+ if (flags& VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
+ virXendError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Cannot modify live config if domain is
inactive"));
+ return -1;
+ }
Should we always error out if _LIVE and inactive, or should we
special-case _CONFIG|_LIVE by silently ignoring the _LIVE flag on
inactive domains?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:47 a.m.
New subject: [libvirt] [PATCH 1/4] xen: Make xenDaemon*DeviceFlags errors less confusing
> When a user calls to
virDomain{Attach,Detach,Update}DeviceFlags() with
> flags == VIR_DOMAIN_DEVICE_MODIFY_LIVE on an inactive guest running on
> an old Xen hypervisor (such as RHEL-5) xend_internal driver reports:
>
> Xend version does not support modifying persistent config
>
> which is pretty confusing since no-one requested to modify persistent
> config.
Hmm - given the recent discussion on vcpus (which is probably what made
you look at this, right?)...
No :-) I was just fixing stuff at this area and noticed this confusing error
message.
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index fce0233..1318bd4 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -3878,6 +3878,12 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char
*xml,
> priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
>
> if (domain->id< 0) {
> + /* Cannot modify live config if domain is inactive */
> + if (flags& VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> + virXendError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("Cannot modify live config if domain is
inactive"));
> + return -1;
> + }
Should we always error out if _LIVE and inactive, or should we
special-case _CONFIG|_LIVE by silently ignoring the _LIVE flag on
inactive domains?
Since there is the _CURRENT variant of the flag, erroring out if _LIVE and
inactive seems like the right thing to do for me. One can always use _CONFIG |
_CURRENT to change everything regardless on current state of the guest.
Anyway, my patch is not aimed at solving the "to error out or not to error
out" question but rather swapping two checks to get errors which are more
logic. I think I should have used more context for the patch to make this
clearer. Sorry about that.
Jirka
9 a.m.
New subject: [libvirt] [PATCH 1/4] xen: Make xenDaemon*DeviceFlags errors less confusing
On 10/04/2010 05:47 AM, Jiri Denemark wrote:
>
> Should we always error out if _LIVE and inactive, or should we
> special-case _CONFIG|_LIVE by silently ignoring the _LIVE flag on
> inactive domains?
Since there is the _CURRENT variant of the flag, erroring out if _LIVE and
inactive seems like the right thing to do for me. One can always use _CONFIG |
_CURRENT to change everything regardless on current state of the guest.
Anyway, my patch is not aimed at solving the "to error out or not to error
out" question but rather swapping two checks to get errors which are more
logic. I think I should have used more context for the patch to make this
clearer. Sorry about that.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:09 p.m.
New subject: [libvirt] [PATCH 2/4] xen: Fix logic bug in xenDaemon*DeviceFlags
---
src/xen/xend_internal.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 1318bd4..4fba6af 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -3904,8 +3904,9 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml,
/* Xen only supports modifying both live and persistent config if
* xendConfigVersion >= 3
*/
- if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
- VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
+ if (priv->xendConfigVersion >= 3 &&
+ (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
_("Xend only supports modifying both live and "
"persistent config"));
@@ -4043,8 +4044,9 @@ xenDaemonUpdateDeviceFlags(virDomainPtr domain, const char *xml,
/* Xen only supports modifying both live and persistent config if
* xendConfigVersion >= 3
*/
- if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
- VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
+ if (priv->xendConfigVersion >= 3 &&
+ (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
_("Xend only supports modifying both live and "
"persistent config"));
@@ -4154,8 +4156,9 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml,
/* Xen only supports modifying both live and persistent config if
* xendConfigVersion >= 3
*/
- if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
- VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
+ if (priv->xendConfigVersion >= 3 &&
+ (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
_("Xend only supports modifying both live and "
"persistent config"));
--
1.7.3.1
4:21 p.m.
New subject: [libvirt] [PATCH 2/4] xen: Fix logic bug in xenDaemon*DeviceFlags
On 10/01/2010 02:09 PM, Jiri Denemark wrote:
---
src/xen/xend_internal.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 1318bd4..4fba6af 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -3904,8 +3904,9 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml,
/* Xen only supports modifying both live and persistent config if
* xendConfigVersion>= 3
*/
- if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
- VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
+ if (priv->xendConfigVersion>= 3&&
+ (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
+ VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
_("Xend only supports modifying both live and "
"persistent config"));
The comment says:
_LIVE|_CONFIG is supported only if version>=3
logically, this tells me nothing about version < 3, nor about setting
one but not both flags.
Meanwhile, the code says:
if version >=3, _LIVE|_CONFIG is the only supported combo
Are we sure this is the right change? What happens with version < 3?
It seems like we need a 12-way table (in fact, this is pretty much what
I ended up resorting to with my vcpus stuff). Here's my shot at it,
from reading the comments (but not actually testing it); once we fix
this attempt to an actual table, then I can answer whether the code
matches the table.
_LIVE _CONFIG _LIVE|_CONFIG
xen 2,running good unsupported unsupported
xen 2,inactive error good error or silently good
xen 3,running good good good
xen 3,inactive error good error or silently good
where the 'error or silently good' depends on our decision of whether an
inactive domain should silently ignore _LIVE.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:28 p.m.
New subject: [libvirt] [PATCH 2/4] xen: Fix logic bug in xenDaemon*DeviceFlags
Eric Blake wrote:
On 10/01/2010 02:09 PM, Jiri Denemark wrote:
> ---
> src/xen/xend_internal.c | 15 +++++++++------
> 1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 1318bd4..4fba6af 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -3904,8 +3904,9 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain,
> const char *xml,
> /* Xen only supports modifying both live and persistent config if
> * xendConfigVersion>= 3
> */
> - if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
> - VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
> + if (priv->xendConfigVersion>= 3&&
> + (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
> + VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
> virXendError(VIR_ERR_OPERATION_INVALID, "%s",
> _("Xend only supports modifying both live and "
> "persistent config"));
The comment says:
_LIVE|_CONFIG is supported only if version>=3
logically, this tells me nothing about version < 3, nor about setting
one but not both flags.
Meanwhile, the code says:
if version >=3, _LIVE|_CONFIG is the only supported combo
Are we sure this is the right change? What happens with version < 3?
It seems like we need a 12-way table (in fact, this is pretty much
what I ended up resorting to with my vcpus stuff).
Yes, good idea.
Here's my shot at it, from reading the comments (but not
actually
testing it); once we fix this attempt to an actual table, then I can
answer whether the code matches the table.
_LIVE _CONFIG _LIVE|_CONFIG
xen 2,running good unsupported unsupported
xen 2,inactive error good error or silently good
xen 3,running good good good
I'm not aware of any way to _only_ hot(un)plug or _only_ change
persistent config in this case. If xend's device_create op is called on
a running domain, the device is hotplugged *and* it is added to the config.
xen 3,inactive error good error or silently good
where the 'error or silently good' depends on our decision of whether
an inactive domain should silently ignore _LIVE.
Otherwise the table looks correct. And IMO "silently good" is acceptable.
Jim
7:01 a.m.
New subject: [libvirt] [PATCH 2/4] xen: Fix logic bug in xenDaemon*DeviceFlags
> ---
> src/xen/xend_internal.c | 15 +++++++++------
> 1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 1318bd4..4fba6af 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -3904,8 +3904,9 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char
*xml,
> /* Xen only supports modifying both live and persistent config if
> * xendConfigVersion>= 3
> */
> - if (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
> - VIR_DOMAIN_DEVICE_MODIFY_CONFIG)) {
> + if (priv->xendConfigVersion>= 3&&
> + (flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
> + VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
> virXendError(VIR_ERR_OPERATION_INVALID, "%s",
> _("Xend only supports modifying both live and
"
> "persistent config"));
The comment says:
_LIVE|_CONFIG is supported only if version>=3
logically, this tells me nothing about version < 3, nor about setting
one but not both flags.
Not really, the comment say that version >= 3 means only _LIVE|_CONFIG is
supported, nothing else.
Meanwhile, the code says:
if version >=3, _LIVE|_CONFIG is the only supported combo
Which is exactly what the comment says IMHO.
Are we sure this is the right change? What happens with version <
3?
With a slightly more context the code looks as follows:
/* Only live config can be changed if xendConfigVersion < 3 */
if (priv->xendConfigVersion < 3 &&
(flags != VIR_DOMAIN_DEVICE_MODIFY_CURRENT &&
flags != VIR_DOMAIN_DEVICE_MODIFY_LIVE)) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
_("Xend version does not support modifying "
"persistent config"));
return -1;
}
/* Xen only supports modifying both live and persistent config if
* xendConfigVersion >= 3
*/
if (priv->xendConfigVersion >= 3 &&
(flags != (VIR_DOMAIN_DEVICE_MODIFY_LIVE |
VIR_DOMAIN_DEVICE_MODIFY_CONFIG))) {
virXendError(VIR_ERR_OPERATION_INVALID, "%s",
_("Xend only supports modifying both live and "
"persistent config"));
return -1;
}
That is, version < 3 is already taken care of in the previous condition. The
only issue is that the code ignores _CURRENT for version >= 3, it should
rather allow _CONFIG && (_LIVE || _CURRENT). Otherwise, it seems correct to
me.
It seems like we need a 12-way table (in fact, this is pretty much
what
I ended up resorting to with my vcpus stuff). Here's my shot at it,
from reading the comments (but not actually testing it); once we fix
this attempt to an actual table, then I can answer whether the code
matches the table.
_LIVE _CONFIG _LIVE|_CONFIG
xen 2,running good unsupported unsupported
xen 2,inactive error good error or silently good
xen 3,running good good good
xen 3,inactive error good error or silently good
Yeah, this is probably a good idea however we shouldn't forget about _CURRENT
which is an equivalent of either _CONFIG or _LIVE depending on current state
of the guest.
Jirka
9:17 a.m.
New subject: [libvirt] [PATCH 2/4] xen: Fix logic bug in xenDaemon*DeviceFlags
On 10/04/2010 06:01 AM, Jiri Denemark wrote:
> It seems like we need a 12-way table (in fact, this is pretty
much what
> I ended up resorting to with my vcpus stuff). Here's my shot at it,
> from reading the comments (but not actually testing it); once we fix
> this attempt to an actual table, then I can answer whether the code
> matches the table.
>
> _LIVE _CONFIG _LIVE|_CONFIG
> xen 2,running good unsupported unsupported
> xen 2,inactive error good error or silently good
> xen 3,running good good good
> xen 3,inactive error good error or silently good
Yeah, this is probably a good idea however we shouldn't forget about _CURRENT
which is an equivalent of either _CONFIG or _LIVE depending on current state
of the guest.
LI: _LIVE
CO: _CONFIG
CU: _CURRENT = active ? _LIVE : _CONFIG
g: good
i: OPERATION_INVALID (in the wrong state)
u: CONFIG_UNSUPPORTED (never possible)
LI CO LI|CO CU LI|CU CO|CU LI|CO|CU
xen 2,running g u u g g u u
xen 2,inactive i u u i i u u
xen 3,running u u g u u g g
xen 3,inactive u u i u i u i
The logic might be rendered easier by starting the function with:
if (!flags)
return -1;
if (flags & _CURRENT)
flags |= running ? _LIVE : _CONFIG;
so that the rest of the function need only operate on _LIVE and _CONFIG
(reducing 2^3==8 flag combinations down to 3).
But, as written, and with your extra context explanations, ACK to patch 2.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:09 p.m.
New subject: [libvirt] [PATCH 3/4] xen: xenXMDomain*DeviceFlags should obey all flags
xenXMDomain*DeviceFlags() silently ignores requests to modify live
configuration of an active guest while still touching its persistent
configuration.
---
src/xen/xm_internal.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
index b9cb4c3..fcc9378 100644
--- a/src/xen/xm_internal.c
+++ b/src/xen/xm_internal.c
@@ -2935,8 +2935,13 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml,
if (domain->conn->flags & VIR_CONNECT_RO)
return -1;
- if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
+
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) ||
+ (domain->id != -1 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT)))
{
+ xenXMError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Xm driver only supports modifying persistent config"));
return -1;
+ }
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
xenUnifiedLock(priv);
@@ -3026,8 +3031,13 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char *xml,
if (domain->conn->flags & VIR_CONNECT_RO)
return -1;
- if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
+
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) ||
+ (domain->id != -1 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT)))
{
+ xenXMError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Xm driver only supports modifying persistent config"));
return -1;
+ }
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
xenUnifiedLock(priv);
--
1.7.3.1
4:50 p.m.
New subject: [libvirt] [PATCH 3/4] xen: xenXMDomain*DeviceFlags should obey all flags
Jiri Denemark wrote:
xenXMDomain*DeviceFlags() silently ignores requests to modify live
configuration of an active guest while still touching its persistent
configuration.
---
src/xen/xm_internal.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c
index b9cb4c3..fcc9378 100644
--- a/src/xen/xm_internal.c
+++ b/src/xen/xm_internal.c
@@ -2935,8 +2935,13 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char
*xml,
if (domain->conn->flags & VIR_CONNECT_RO)
return -1;
- if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
+
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) ||
+ (domain->id != -1 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT)))
{
+ xenXMError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Xm driver only supports modifying persistent config"));
return -1;
+ }
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
xenUnifiedLock(priv);
@@ -3026,8 +3031,13 @@ xenXMDomainDetachDeviceFlags(virDomainPtr domain, const char
*xml,
if (domain->conn->flags & VIR_CONNECT_RO)
return -1;
- if (domain->id != -1 && !(flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG))
+
+ if ((flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) ||
+ (domain->id != -1 && (flags & VIR_DOMAIN_DEVICE_MODIFY_CURRENT)))
{
+ xenXMError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("Xm driver only supports modifying persistent config"));
return -1;
+ }
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
xenUnifiedLock(priv);
ACK.
9:41 a.m.
New subject: [libvirt] [PATCH 3/4] xen: xenXMDomain*DeviceFlags should obey all flags
On 10/01/2010 02:09 PM, Jiri Denemark wrote:
xenXMDomain*DeviceFlags() silently ignores requests to modify live
configuration of an active guest while still touching its persistent
configuration.
---
src/xen/xm_internal.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:09 p.m.
New subject: [libvirt] [PATCH 4/4] xen: Fix virDomain{At,De}tachDevice
According to API documentation virDomain{At,De}tachDevice calls are
supposed to only work on active guests for device hotplug. For anything
beyond that, their *Flags variants have to be used.
Despite the variant which was acked on libvirt mailing list
(https://www.redhat.com/archives/libvir-list/2010-January/msg00385.html)
commit ed9c14a7ef86d7a45a6d57cbfee5410fca428633 (by Jim Fehlig)
introduced automagic behavior of these API calls for xen driver. Since
January, these calls always change persistent configuration of a guest
and if the guest is currently active, they also hot(un)plug the device.
That change didn't follow API documentation and also broke device
hot(un)plug for older xend implementations which do not support changing
persistent configuration of a guest and hot(un)plugging in one step.
This patch should not break anything for active guests. On the other
hand, changing inactive guests is not supported any more.
---
src/xen/xen_driver.c | 24 ++++++++++++++++++------
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 56ba41b..e9eeab9 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -1437,10 +1437,16 @@ xenUnifiedDomainAttachDevice (virDomainPtr dom, const char *xml)
{
GET_PRIVATE(dom->conn);
int i;
- unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+ unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE;
- if (dom->id >= 0)
- flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+ /*
+ * HACK: xend with xendConfigVersion >= 3 does not support changing live
+ * config without touching persistent config, we add the extra flag here
+ * to make this API work
+ */
+ if (priv->opened[XEN_UNIFIED_XEND_OFFSET] &&
+ priv->xendConfigVersion >= 3)
+ flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i)
if (priv->opened[i] && drivers[i]->domainAttachDeviceFlags
&&
@@ -1470,10 +1476,16 @@ xenUnifiedDomainDetachDevice (virDomainPtr dom, const char *xml)
{
GET_PRIVATE(dom->conn);
int i;
- unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+ unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE;
- if (dom->id >= 0)
- flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+ /*
+ * HACK: xend with xendConfigVersion >= 3 does not support changing live
+ * config without touching persistent config, we add the extra flag here
+ * to make this API work
+ */
+ if (priv->opened[XEN_UNIFIED_XEND_OFFSET] &&
+ priv->xendConfigVersion >= 3)
+ flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i)
if (priv->opened[i] && drivers[i]->domainDetachDeviceFlags
&&
--
1.7.3.1
5:22 p.m.
New subject: [libvirt] [PATCH 4/4] xen: Fix virDomain{At,De}tachDevice
Jiri Denemark wrote:
According to API documentation virDomain{At,De}tachDevice calls are
supposed to only work on active guests for device hotplug. For anything
beyond that, their *Flags variants have to be used.
Despite the variant which was acked on libvirt mailing list
(https://www.redhat.com/archives/libvir-list/2010-January/msg00385.html)
commit ed9c14a7ef86d7a45a6d57cbfee5410fca428633 (by Jim Fehlig)
introduced automagic behavior of these API calls for xen driver. Since
January, these calls always change persistent configuration of a guest
and if the guest is currently active, they also hot(un)plug the device.
That change didn't follow API documentation and also broke device
hot(un)plug for older xend implementations which do not support changing
persistent configuration of a guest and hot(un)plugging in one step.
I only tested as far back as Xen 3.2.1, which is unfortunately still
xendConfigVersion 4. This (nearly useless) version value was last
bumped by danpb over 3 years ago -
http://xenbits.xensource.com/xen-unstable.hg?rev/887fa548f650 :-(.
This patch should not break anything for active guests. On the other
hand, changing inactive guests is not supported any more.
---
src/xen/xen_driver.c | 24 ++++++++++++++++++------
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index 56ba41b..e9eeab9 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -1437,10 +1437,16 @@ xenUnifiedDomainAttachDevice (virDomainPtr dom, const char *xml)
{
GET_PRIVATE(dom->conn);
int i;
- unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+ unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE;
- if (dom->id >= 0)
- flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+ /*
+ * HACK: xend with xendConfigVersion >= 3 does not support changing live
+ * config without touching persistent config, we add the extra flag here
+ * to make this API work
+ */
+ if (priv->opened[XEN_UNIFIED_XEND_OFFSET] &&
+ priv->xendConfigVersion >= 3)
+ flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i)
if (priv->opened[i] && drivers[i]->domainAttachDeviceFlags
&&
@@ -1470,10 +1476,16 @@ xenUnifiedDomainDetachDevice (virDomainPtr dom, const char *xml)
{
GET_PRIVATE(dom->conn);
int i;
- unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
+ unsigned int flags = VIR_DOMAIN_DEVICE_MODIFY_LIVE;
- if (dom->id >= 0)
- flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE;
+ /*
+ * HACK: xend with xendConfigVersion >= 3 does not support changing live
+ * config without touching persistent config, we add the extra flag here
+ * to make this API work
+ */
+ if (priv->opened[XEN_UNIFIED_XEND_OFFSET] &&
+ priv->xendConfigVersion >= 3)
+ flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG;
for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i)
if (priv->opened[i] && drivers[i]->domainDetachDeviceFlags
&&
ACK. I have tested this on Xen 4.0.0 setup, both active and inactive pv
and hvm domUs.
Regards,
Jim
9:42 a.m.
New subject: [libvirt] [PATCH 4/4] xen: Fix virDomain{At,De}tachDevice
On 10/01/2010 02:09 PM, Jiri Denemark wrote:
According to API documentation virDomain{At,De}tachDevice calls are
supposed to only work on active guests for device hotplug. For anything
beyond that, their *Flags variants have to be used.
Despite the variant which was acked on libvirt mailing list
(https://www.redhat.com/archives/libvir-list/2010-January/msg00385.html)
commit ed9c14a7ef86d7a45a6d57cbfee5410fca428633 (by Jim Fehlig)
introduced automagic behavior of these API calls for xen driver. Since
January, these calls always change persistent configuration of a guest
and if the guest is currently active, they also hot(un)plug the device.
That change didn't follow API documentation and also broke device
hot(un)plug for older xend implementations which do not support changing
persistent configuration of a guest and hot(un)plugging in one step.
This patch should not break anything for active guests. On the other
hand, changing inactive guests is not supported any more.
---
src/xen/xen_driver.c | 24 ++++++++++++++++++------
1 files changed, 18 insertions(+), 6 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:27 a.m.
xen: Make xenDaemon*DeviceFlags errors less confusing
xen: Fix logic bug in xenDaemon*DeviceFlags
xen: xenXMDomain*DeviceFlags should obey all flags
xen: Fix virDomain{At,De}tachDevice
src/xen/xen_driver.c | 24 ++++++++++++++++-----
src/xen/xend_internal.c | 51 ++++++++++++++++++++++++----------------------
src/xen/xm_internal.c | 14 +++++++++++-
3 files changed, 57 insertions(+), 32 deletions(-)
Eric and Jim, thanks for the review and additional testing.
I pushed this series.
Jirka
5156
days inactive
5167
days old
16 comments
3 participants
participants (3)
-
Eric Blake -
Jim Fehlig -
Jiri Denemark