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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxx>

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux