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

[Markus Armbruster]

Michal Privoznik <address@hidden> writes:

> On 09/06/2017 07:59 AM, Markus Armbruster wrote:
>> 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);
>
> I'm not a fan of this, but I'm not a qemu developer so I don't know what
> your coding style is (if any for this case). However, since this switch
> is over an enum, compiler will scream if a new value is introduced to
> the enum and not handled in the switch. IMO it's useful because when I'm
> adding new value I can immediately see what other places I need to consider.

Two separate purposes: catch invalid state, catch forgotten code update.
I can't see how to serve both.  Your patch picks the latter.

> Also, yeah, I am going to rename the enum in v3.

Thanks!