Re: [Qemu-devel] [PATCH v2] watchdog: Allow setting action on the fly

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/05/2017 08:22 AM, Michal Privoznik wrote:
>> Currently, the only time that users can set watchdog action is at
>> the start as all we expose is this -watchdog-action command line
>> argument. This is suboptimal when users want to plug the device
>> later via monitor. Alternatively, they might want to change the
>> action for already existing device on the fly.
>> 
>> At the same time, drop local redefinition of the actions eum in
>
> s/eum/enum/
>
>> watchdog.h in favour of the ones defined in schema.
>> 
>> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>> 
>> Signed-off-by: Michal Privoznik <address@hidden>
>> ---
>
>> @@ -77,27 +80,17 @@ int select_watchdog(const char *p)
>>  
>>  int select_watchdog_action(const char *p)
>>  {
>
>> +    int action;
>>  
>> +    action = qapi_enum_parse(WatchdogExpirationAction_lookup, p,
>> +                             WATCHDOG_EXPIRATION_ACTION__MAX, -1, NULL);
>
> Semantic conflict; you need to rebase now that the qapi enum lookup code
> has landed (see commit ebf677c8 and parents)
>
>> +
>> +    case WATCHDOG_EXPIRATION_ACTION__MAX:
>> +        /* keep gcc happy */
>> +        break;
>
> Could use g_assert_not_reached() here instead.

Catches one out of approximately 2^64 invalid values.  To catches all,
and in fewer words:

        default:
            assert(0);

>> +++ b/qapi-schema.json
>> @@ -6539,3 +6539,12 @@
>>  # Since 2.9
>>  ##
>>  { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
>> +
>> +##
>> +# @watchdog-set-action:
>> +#
>> +# Set watchdog action
>> +#
>> +# Since 2.11
>> +##
>> +{ 'command': 'watchdog-set-action', 'data' : {'action': 
>> 'WatchdogExpirationAction'} }
>
> Can a machine have more than one watchdog device, in which case you'd
> want a device name to select which watchdog gets which action?  But the
> command-line version that you are replacing seems to be global, so I
> guess you're okay with this interface.

For better or worse, all watchdogs share the action, see
hw/watchdog/watchdog.c.

Watchdogs predate qdev.  Commit 9dd986c lets you configure up to one,
with two command line options: --watchdog and --watchdog-action.  In a
way, --watchdog configures the frontend, and --watchdog-action the
backend (having watchdog.c in hw/watchdog/ contradicts this view,
though).

Since the frontends got qdevified (commit 09aaa16), you can configure
any number of watchdog frontends, but still only one backend shared by
all.  Unsharing the backend wasn't worth the trouble, because having
multiple watchdogs would be weird, and multiple watchdogs with different
actions would be weirder.  Trouble would include maintaining command
line backward compatibility, as always.

Of course, if we *want* to unshare the backend, we should do it *before*
we add QMP backward compatibility to the trouble.