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

[Michal Privoznik]

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.

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

Michal