Re: [PATCH V4 0/5] Introduce Advanced Watch Dog module

On 04/03/20 09:06, Zhang, Chen wrote: My main comment about this series is that it's not clear why it is needed and how to use it. The documentation includes a demo, but no description of what is an awd_node, a notification_node and an opt_script. I can more or less understand the notification_node and opt_script role from the documentation, but not entirely because, for example, the two-host demo has hardcoded IP addresses without saying which host is which IP address. The documentation does not describe the protocol, which is absolutely necessary, and does not describe _why_ the protocol was designed like that. Without such documentation it's not clear if, for example, the watchdog protocol could be implemented as QMP commands (e.g. start-watchdog, stop-watchdog, notify-watchdog). Another possibility could be to use the systemd watchdog protocol, which consists of essentially three commands (WATCHDOG=1, WATCHDOG=trigger, WATCHDOG_USEC=...) which are transmitted as datagrams. Documentation is important for reviewers to judge the merits of the protocol without (or before) diving into the code. In the demo, the opt_script mechanism is currently using the "human" monitor as opposed to QMP. The human monitor interface is not stable and not meant for consumption by management interface. It is not clear if this is just a sample usage, and in practice the notification_node would be outside of QEMU, or not. In general I would prefer to have the script as an optional feature, and report the triggering of the watchdog via QMP events. Paolo