[libvirt] [PATCH 3/4] qemu: hot-plug of watchdog
Michal Privoznik
mprivozn at redhat.com
Fri Sep 22 14:31:43 UTC 2017
On 09/12/2017 04:23 PM, John Ferlan wrote:
>
>
> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>
>> Once again, 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 | 15 ++++-
>> src/qemu/qemu_alias.h | 2 +
>> src/qemu/qemu_command.c | 2 +-
>> src/qemu/qemu_command.h | 3 +
>> src/qemu/qemu_driver.c | 10 +++-
>> src/qemu/qemu_hotplug.c | 64 ++++++++++++++++++++++
>> 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 | 55 +++++++++++++++++++
>> 14 files changed, 205 insertions(+), 4 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..63f69e80f 100644
>> --- a/src/qemu/qemu_alias.c
>> +++ b/src/qemu/qemu_alias.c
>> @@ -405,6 +405,19 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>> }
>>
>>
>> +int
>> +qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog)
>> +{
>> + const int idx = 0;
>> +
>> + /* Currently, there's just one watchdog per domain */
>> +
>> + if (virAsprintf(&watchdog->info.alias, "watchdog%d", idx) < 0)
>
> There's really no need for @idx... s/idx/0/ works fine.
>
> Although if multiple watchdogs were ever to be implemented, then using
> the watchdog->action could perhaps be a mechanism to have a watchdog for
> each type of action. Of course that breaks down for an existing domain
> on an old version where a new libvirtd gets added and hot unplug is
> attempted (since the running domain would always have watchdog0). Oh
> well.... It was a thought.
>
>> + return -1;
>> + return 0;
>> +}
>> +
>> +
>> int
>> qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>> {
>> @@ -482,7 +495,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>> return -1;
>> }
>> if (def->watchdog) {
>> - if (virAsprintf(&def->watchdog->info.alias, "watchdog%d", 0) < 0)
>> + if (qemuAssignDeviceWatchdogAlias(def->watchdog) < 0)
>> return -1;
>> }
>> if (def->memballoon) {
>> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
>> index 300fd4de5..652ffea0c 100644
>> --- a/src/qemu/qemu_alias.h
>> +++ b/src/qemu/qemu_alias.h
>> @@ -65,6 +65,8 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>> virDomainShmemDefPtr shmem,
>> int idx);
>>
>> +int qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog);
>> +
>> int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
>>
>> int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 9a27987d4..eb9df044b 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3921,7 +3921,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>> }
>>
>>
>> -static char *
>> +char *
>> qemuBuildWatchdogDevStr(const virDomainDef *def,
>> virDomainWatchdogDefPtr dev,
>> virQEMUCapsPtr qemuCaps)
>> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
>> index 6fbfb3e5f..4b504f685 100644
>> --- a/src/qemu/qemu_command.h
>> +++ b/src/qemu/qemu_command.h
>> @@ -206,5 +206,8 @@ char *qemuBuildShmemDevStr(virDomainDefPtr def,
>> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>
>
> Only one space between... Although, I see commit id '06524fd5' a few
> extra blank spaces. We can use this opportunity to clean them up
> though... Just be sure there's an extra blank line before the #endif
> though (helps w/ readability at least for me).
>
>>
>> +char * qemuBuildWatchdogDevStr(const virDomainDef *def,
>> + virDomainWatchdogDefPtr dev,
>> + virQEMUCapsPtr qemuCaps);
>
> You have an extra space between * and qemu... Of course affects spacing
> of subsequent lines too. Could always go with the same format as .c:
>
> char *
> qemuBuildWatchdogDevStr(const virDomainDef *def,
> virDomainWatchdogDefPtr dev,
> virQEMUCapsPtr qemuCaps);
>
>
>
> My only other note is I really wish qemu_command.h was ordered the same
> way as qemu_command.c, but that's a different problem. IOW: Why add to
> the end, but that's one of those type-A type problems.
>
>>
>> #endif /* __QEMU_COMMAND_H__*/
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 85b4be360..626148dba 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -7593,12 +7593,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>> }
>> break;
>>
>> + case VIR_DOMAIN_DEVICE_WATCHDOG:
>> + ret = qemuDomainAttachWatchdog(driver, vm,
>> + dev->data.watchdog);
>> + if (!ret) {
>
> I wish all the callers used "if (ret == 0)", but I won't complain ;-)
>
>> + alias = dev->data.watchdog->info.alias;
>> + dev->data.watchdog = NULL;
>> + }
>> + break;
>> +
>> case VIR_DOMAIN_DEVICE_NONE:
>> 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 b365078ec..989146eb9 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -2866,6 +2866,70 @@ 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;
>> + 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->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> + if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0)
>> + goto cleanup;
>> + releaseAddress = true;
>> + }
>> +
>> + /* 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;
>> +
>
> I see the command line building has similar code, but also has a check
> that actualAction actually *TypeToString properly.
That is really useless. How can we successfully parse action but fail to
format it back?
>
>> + qemuDomainObjEnterMonitor(driver, vm);
>> +
>> + rv = qemuMonitorSetWatchdogAction(priv->mon, actualAction);
>
> Any reason to not pass a const char *action here instead? Allowing this
> code to check validity rather than failing lower in the stack.
We can fail lower in the stack but not because of bad conversion from
enum to string. But okay, I can change this to const char *action. We
use both approaches, so one can argue either way.
>
> I also think perhaps there could be a "common" API that both
> qemu_command and qemu_hotplug could call in order to return the action
> string so that you remove the duplicated code.
Well the only duplicated part is the check where action is overwritten.
I don't think it's that bad.
>
> My only other comment is that it seems a bit strange to adjust the
> action of something that isn't yet added to the domain. That is the
> ordering of altering the action, then adding the watchdog doesn't seem
> right. The command line has the "-device" first, then the "action"
> second, that is I see from the *.args file:
>
> ...
> -device ib700,id=watchdog0 \
> -watchdog-action pause \
> ...
>
> which if qemu is reading the order would seem to imply the device is
> added, then the action is applied.
That's just coincidence IMO. The true reason was that I don't have to
try unplug the watchdog if setting action fails.
>
>> +
>> + if (rv >= 0)
>
> This only ever returns -1 or 0, right?
Yep. But in general, 0 or a positive value are considered as success.
>
>> + rv = qemuMonitorAddDevice(priv->mon, watchdogstr);
>> +
>> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>> + releaseAddress = false;
>> + goto cleanup;
>> + }
>> +
>> + if (rv < 0)
>> + goto cleanup;
>
> Why no virDomainAuditWatchdog type code?
Ah, good point. There's no virDomainAuditWatchdog(), but there should
be! I'll post another version where I'll introduce the audit function as
the first patch.
>
>> +
>> + releaseAddress = false;
>> + vm->def->watchdog = watchdog;
>> + ret = 0;
>> +
>> + cleanup:
>> + if (releaseAddress)
>> + qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>> + VIR_FREE(watchdogstr);
>> + return ret;
>> +}
>> +
>> +
>> static int
>> qemuDomainChangeNetBridge(virDomainObjPtr vm,
>> virDomainNetDefPtr olddev,
>> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
>> index 985c6733f..a9dfd8f7a 100644
>> --- a/src/qemu/qemu_hotplug.h
>> +++ b/src/qemu/qemu_hotplug.h
>> @@ -80,6 +80,9 @@ int qemuDomainAttachHostDevice(virConnectPtr conn,
>> int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
>> virDomainObjPtr vm,
>> virDomainShmemDefPtr shmem);
>> +int qemuDomainAttachWatchdog(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + virDomainWatchdogDefPtr watchdog);
>> int qemuDomainFindGraphicsIndex(virDomainDefPtr def,
>> virDomainGraphicsDefPtr dev);
>> int qemuDomainAttachMemory(virQEMUDriverPtr driver,
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 19082d8bf..5d3f9df47 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -4343,3 +4343,15 @@ qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info)
>>
>> VIR_FREE(info);
>> }
>> +
>> +
>> +int
>> +qemuMonitorSetWatchdogAction(qemuMonitorPtr mon,
>> + virDomainWatchdogAction watchdogAction)
>> +{
>> + VIR_DEBUG("watchdogAction=%d", watchdogAction);
>> +
>> + QEMU_CHECK_MONITOR_JSON(mon);
>> +
>> + return qemuMonitorJSONSetWatchdogAction(mon, watchdogAction);
>> +}
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index 9805a3390..b8c9409e5 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -1132,4 +1132,6 @@ int qemuMonitorSetBlockThreshold(qemuMonitorPtr mon,
>>
>> virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon);
>>
>> +int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon,
>> + virDomainWatchdogAction watchdogAction);
>
> Prefer to see that extra blank line here...
>
>> #endif /* QEMU_MONITOR_H */
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index df5fb7c8f..02ba701e6 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -7679,3 +7679,31 @@ qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon)
>>
>> return ret;
>> }
>> +
>> +
>> +int
>> +qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon,
>> + virDomainWatchdogAction watchdogAction)
>> +{
>> + virJSONValuePtr cmd;
>> + virJSONValuePtr reply = NULL;
>> + const char *action = virDomainWatchdogActionTypeToString(watchdogAction);
>> + int ret = -1;
>
> Command line building checks !action....
>
> I would think this function should take a "const char *action" and the
> caller handle the error though.
>
>> +
>> + if (!(cmd = qemuMonitorJSONMakeCommand("watchdog-set-action",
>> + "s:action", action,
>> + NULL)))
>> + return -1;
>> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
>> + goto cleanup;
>> +
>> + if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>> + goto cleanup;
>> +
>> + ret = 0;
>> +
>> + cleanup:
>> + virJSONValueFree(cmd);
>> + virJSONValueFree(reply);
>> + return ret;
>> +}
>> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
>> index 7462967b5..a7a43d7b8 100644
>> --- a/src/qemu/qemu_monitor_json.h
>> +++ b/src/qemu/qemu_monitor_json.h
>> @@ -524,4 +524,7 @@ int qemuMonitorJSONSetBlockThreshold(qemuMonitorPtr mon,
>> virJSONValuePtr qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon)
>> ATTRIBUTE_NONNULL(1);
>>
>> +int qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon,
>> + virDomainWatchdogAction watchdogAction)
>> + ATTRIBUTE_NONNULL(1);
>
> Likewise with the extra blank line.
>
>> #endif /* QEMU_MONITOR_JSON_H */
>> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
>> index 23a498617..b02ae8034 100644
>> --- a/tests/qemuhotplugtest.c
>> +++ b/tests/qemuhotplugtest.c
>> @@ -127,6 +127,9 @@ testQemuHotplugAttach(virDomainObjPtr vm,
>> case VIR_DOMAIN_DEVICE_SHMEM:
>> ret = qemuDomainAttachShmemDevice(&driver, vm, dev->data.shmem);
>> break;
>> + case VIR_DOMAIN_DEVICE_WATCHDOG:
>> + ret = qemuDomainAttachWatchdog(&driver, vm, dev->data.watchdog);
>> + break;
>> default:
>> VIR_TEST_VERBOSE("device type '%s' cannot be attached\n",
>> virDomainDeviceTypeToString(dev->type));
>> @@ -811,10 +814,14 @@ mymain(void)
>> "device_del", QMP_OK,
>> "object-del", QMP_OK);
>> DO_TEST_ATTACH("base-live+disk-scsi-wwn",
>> - "disk-scsi-duplicate-wwn", false, true,
>> + "disk-scsi-duplicate-wwn", false, false,
>
> This would seem to be unrelated... and should be separate.
Not really. We don't want to keep the device (=disk) in the domain
because then the test case I'm adding would need to look different (with
that disk in, which would be insane).
Michal
More information about the libvir-list
mailing list