[libvirt] [PATCH 4/4] qemu: hot-unplug of watchdog

[Michal Privoznik]

On 09/26/2017 12:30 PM, Daniel P. Berrange wrote:
> On Tue, Sep 26, 2017 at 11:38:26AM +0200, Michal Privoznik wrote:
>> On 09/12/2017 04:29 PM, John Ferlan wrote:
>>>
>>>
>>> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>>  src/qemu/qemu_driver.c                             |  4 +-
>>>>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
>>>>  src/qemu/qemu_hotplug.h                            |  3 ++
>>>>  tests/qemuhotplugtest.c                            |  7 ++-
>>>>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
>>>>  5 files changed, 76 insertions(+), 2 deletions(-)
>>>>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 626148dba..4c636b956 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>>>>      case VIR_DOMAIN_DEVICE_SHMEM:
>>>>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
>>>>          break;
>>>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>>> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
>>>> +        break;
>>>>  
>>>>      case VIR_DOMAIN_DEVICE_FS:
>>>>      case VIR_DOMAIN_DEVICE_INPUT:
>>>>      case VIR_DOMAIN_DEVICE_SOUND:
>>>>      case VIR_DOMAIN_DEVICE_VIDEO:
>>>> -    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>>>      case VIR_DOMAIN_DEVICE_GRAPHICS:
>>>>      case VIR_DOMAIN_DEVICE_HUB:
>>>>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>>> index 989146eb9..44472ce9f 100644
>>>> --- a/src/qemu/qemu_hotplug.c
>>>> +++ b/src/qemu/qemu_hotplug.c
>>>> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
>>>>  }
>>>>  
>>>>  
>>>> +static int
>>>> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
>>>> +                         virDomainObjPtr vm,
>>>> +                         virDomainWatchdogDefPtr watchdog)
>>>> +{
>>>> +    virObjectEventPtr event = NULL;
>>>> +
>>>> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",
>>>
>>> Is "Removing watchdog watchdog0 from ..." a bit superfluous?
>>>
>>> Perhaps just "Removing '%s' from ..."
>>>
>>>> +              watchdog->info.alias, vm, vm->def->name);
>>>> +
>>>
>>> This would/could be the place for the virDomainAuditWatchdog for
>>> "detach" too.
>>>
>>>> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
>>>> +    qemuDomainEventQueue(driver, event);
>>>> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>>>> +    virDomainWatchdogDefFree(vm->def->watchdog);
>>>> +    vm->def->watchdog = NULL;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +
>>>>  int
>>>>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>>>>                         virDomainObjPtr vm,
>>>> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>>>>  }
>>>>  
>>>>  
>>>> +int
>>>> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>>>> +                         virDomainObjPtr vm,
>>>> +                         virDomainWatchdogDefPtr dev)
>>>> +{
>>>> +    int ret = -1;
>>>> +    virDomainWatchdogDefPtr watchdog;
>>>
>>> Why not watchdog = vm->def->watchdog; here?
>>>
>>>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>>>> +
>>>> +    /* While domains can have up to one watchdog, the one supplied by user
>>>
>>> s/by/by the/
>>>
>>>> +     * doesn't necessarily match the one domain has. Refuse to detach in such
>>>> +     * case. */
>>>> +    if (!(vm->def->watchdog &&
>>>> +          STREQ_NULLABLE(dev->info.alias,
>>>> +                         vm->def->watchdog->info.alias))) {
>>>
>>> if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))
>>>
>>> Trying to think why NULLABLE would be necessary... So it seems it's
>>> required that that input XML has the alias - something not quite right
>>> with that...
>>
>> Is that so? We match devices based on their alias in a lot of places.
>> Users are expected to pass the full device XML anyway. So it's up to us
>> how we pick the right device to be detached. For instance, when
>> detaching a vNIC we match MAC addresses and ignore the rest, since MAC
>> identifies the device uniquely. So for instance, if the detach XML
>> provided by user has <bandwidth/> set but the one in domain doesn't have
>> it, we detach the device anyway. One can argue this is a buggy
>> behaviour. But I just don't think we should care. There's a line we have
>> to draw between protecting users from themselves and too complex and
>> mostly useless code. So we've documented that users are supposed to pass
>> the device XML as is in the domain XML.
> 
> We can protect ourselves from the danger of user passing incomplete device
> XML by not using the passed in device XML directly.
> 
> IOW, once we parse the XML to create our virDomainDevicePtr instance,
> lets say we want the dev->disk virDomainDiskDefPtr to unplug a disk.
> Don't use the the dev->disk pointer in the unplug code. Instead use
> the dev->disk to lookup the def->disk[N] we want from virDomaniDefPtr
> and then use that virDomainDiskDefPtr instead. That way our code doesn't
> have to be paranoid about incompletely specified device configs. This
> is safer because we'll inevitably forget to be paranoid enough in places
> which leaves us vulnerable to crashes / bugs.
> 

Yup. That's exactly what we do. And while in fact using our own
definition of the device, the one provided by user is not taken fully
into consideration. Which is good. I'm just saying that to explain why I
think that alias is just enough for watchdog detach. Look at my code,
I'm using alias just to lookup the config stored in domain def.

Michal