[libvirt] [PATCH 2/4] xen: Fix logic bug in xenDaemon*DeviceFlags

[Jiri Denemark]

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