[libvirt] [PATCH v3 2/3] qemu: hot-plug of watchdog

[John Ferlan]

On 09/27/2017 08:12 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
> 
> Since domain can have at most one watchdog it simplifies things a
> bit. However, since we must be able to set the watchdog action as
> well, new monitor command needs to be used.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_alias.c                              | 13 +++-
>  src/qemu/qemu_alias.h                              |  2 +
>  src/qemu/qemu_command.c                            |  2 +-
>  src/qemu/qemu_command.h                            |  4 +-
>  src/qemu/qemu_driver.c                             | 10 ++-
>  src/qemu/qemu_hotplug.c                            | 72 ++++++++++++++++++++++
>  src/qemu/qemu_hotplug.h                            |  3 +
>  src/qemu/qemu_monitor.c                            | 12 ++++
>  src/qemu/qemu_monitor.h                            |  2 +
>  src/qemu/qemu_monitor_json.c                       | 28 +++++++++
>  src/qemu/qemu_monitor_json.h                       |  3 +
>  tests/qemuhotplugtest.c                            |  9 ++-
>  .../qemuhotplug-watchdog.xml                       |  1 +
>  .../qemuhotplug-base-live+watchdog.xml             | 56 +++++++++++++++++
>  14 files changed, 212 insertions(+), 5 deletions(-)
>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml
>  create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml
> 
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index 914b2b94d..72df1083f 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -405,6 +405,17 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>  }
>  
>  
> +int
> +qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog)
> +{
> +    /* Currently, there's just one watchdog per domain */
> +
> +    if (VIR_STRDUP(watchdog->info.alias, "watchdog0") < 0)
> +        return -1;
> +    return 0;
> +}
> +
> +

Not trying to increase the patch count for review purposes, but this
easily could have been a separate patch ;-)

[...]

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 4913e18e6..11afa7ec6 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2836,6 +2836,78 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> +int
> +qemuDomainAttachWatchdog(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         virDomainWatchdogDefPtr watchdog)
> +{
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_WATCHDOG, { .watchdog = watchdog } };
> +    virDomainWatchdogAction actualAction = watchdog->action;
> +    const char *actionStr = NULL;
> +    char *watchdogstr = NULL;
> +    bool releaseAddress = false;
> +    int rv;
> +
> +    if (vm->def->watchdog) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("domain already has a watchdog"));
> +        return -1;
> +    }
> +
> +    if (qemuAssignDeviceWatchdogAlias(watchdog) < 0)
> +        return -1;
> +
> +    if (!(watchdogstr = qemuBuildWatchdogDevStr(vm->def, watchdog, priv->qemuCaps)))
> +        return -1;
> +
> +    if (watchdog->model == VIR_DOMAIN_WATCHDOG_MODEL_I6300ESB) {
> +        if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0)
> +            goto cleanup;
> +        releaseAddress = true;
> +    } else {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                       _("hotplug of watchdog of model %s is not supported"),
> +                       virDomainWatchdogModelTypeToString(watchdog->model));
> +        goto cleanup;
> +    }
> +
> +    /* QEMU doesn't have a 'dump' action; we tell qemu to 'pause', then
> +       libvirt listens for the watchdog event, and we perform the dump
> +       ourselves. so convert 'dump' to 'pause' for the qemu cli */
> +    if (actualAction == VIR_DOMAIN_WATCHDOG_ACTION_DUMP)
> +        actualAction = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE;
> +
> +    actionStr = virDomainWatchdogActionTypeToString(actualAction);
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +
> +    rv = qemuMonitorSetWatchdogAction(priv->mon, actionStr);

Something that didn't dawn on me for previous review... Where's the
qemuCaps for "watchdog-set-action" that (ahem) you introduced into qemu
2.11?

No sense in even trying if the command is not there.  Not personally
trying to increase the patches to review, but seems there could be some
extra separation possible (alias, caps, monitor/json, hotplug, unplug).

Additionally, is there a minimum version that allows hot-plug of a
watchdog device that we need to concern ourselves with handling?

I'm fine with the rest of the overall design/concepts, I just think you
need to split up a wee bit more and of course add the caps check....

John

> +
> +    if (rv >= 0)
> +        rv = qemuMonitorAddDevice(priv->mon, watchdogstr);
> +
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> +        releaseAddress = false;
> +        goto cleanup;
> +    }
> +
> +    if (rv < 0)
> +        goto cleanup;
> +
> +    releaseAddress = false;
> +    vm->def->watchdog = watchdog;
> +    ret = 0;
> +
> + cleanup:
> +    if (releaseAddress)
> +        qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
> +    VIR_FREE(watchdogstr);
> +    return ret;
> +}
> +
> +


[...]