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

[Jim Fehlig]

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