> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list