[libvirt] [PATCH 0/4] xen: Fix device hot(un)plug
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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
--- 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
--- 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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.
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
participants (3)
-
Eric Blake -
Jim Fehlig -
Jiri Denemark