[libvirt] [PATCH v3 2/3] qemu: hot-plug of watchdog
John Ferlan
jferlan at redhat.com
Thu Oct 5 11:48:03 UTC 2017
On 10/05/2017 04:07 AM, Michal Privoznik wrote:
> On 10/04/2017 11:20 PM, John Ferlan wrote:
>>
>>
>> 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?
>
> Do we do that? IMO, if the command is not there, we just error out.
> There are plenty of examples, for instance:
> qemuMonitorJSONGetMemoryDeviceInfo(). If the command is not there, -2 is
> returned. Not that the caller would care.
Well - guess I just assumed... Too many commands to know about I guess
in order to know whether we had/used ones in that manner. I suppose the
"usual" manner of ensuring that a command option exists before using and
supplying a (nicer) message about that "This QEMU doesn't support ..."
as opposed to what I assume will be "unable to execute QEMU command...".
> I mean, we might have caps for commands, but I guess that's for
> different reason. For instance, we have QEMU_CAPS_WAKEUP. But this
> capability exist so that we know upfront if the command is available and
> don't learn that in the middle after we've suspended the domain and have
> no way of waking it up. So that's slightly different use case, isn't it?
> I view qemuCaps as helper for cmd line generation mostly.
>
>>
>> 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 don't think so. If qemu is old enough that it lacks
> watchdog-set-action we don't even get to the point of trying to hotplug
> the watchdog. and if it is new enough that it as the command it supports
> watchdog hotplug already.
>
Duh, the question came as a result of hot unplug where I started
thinking too much, but without a hot unplug, then you've got no hot
plug. Same 'concept' applies - failure will come from QEMU though (one
would think/hope).
>>
>> 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....
>
> Well, I can split it if you want me to, but:
>
> a) in the end the code will look the same,
> b) it doesn't make sense for somebody to backport just a part of it.
> They'll backport either all of them or none. They might as well just
> backport this one. Or not.
>
> Michal
>
Hey - I used those arguments in my head many times ;-) - perhaps even
the dog has heard them a few times. I suppose since there's no reason
to go back and rework in order to add a capability for the command, then
no need to deal with splitting up any more, so...
Reviewed-by: John Ferlan <jferlan at redhat.com>
John
More information about the libvir-list
mailing list