--- 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