Re: [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.