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

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(a)redhat.com&gt; >>> --- >>> 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 >>>

>> 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(a)redhat.com&gt;