Re: [Qemu-devel] [PATCH v4 2/3] watchdog.h: Drop local redefinition of actions enum

[Michal Privoznik]

On 09/06/2017 07:25 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 09/06/2017 06:24 AM, Michal Privoznik wrote:
>>> We already have enum that enumerates all the action that a
>>
>> s/action/actions/
>>
>>> watchdog can take when hitting its timeout: WatchdogAction.
>>> Use that instead of inventing our own.
>>>
>>> Signed-off-by: Michal Privoznik <address@hidden>
>>> ---
>>
>>> @@ -77,27 +77,16 @@ int select_watchdog(const char *p)
>>>  
>>>  int select_watchdog_action(const char *p)
>>>  {
>>> -    if (strcasecmp(p, "reset") == 0)
>>> -        watchdog_action = WDT_RESET;
>>
>> The old code was case-insensitive,
>>
>>> +    action = qapi_enum_parse(&WatchdogAction_lookup, p, -1, NULL);
>>
>> the new code is not.  Do we care?  (I don't, but we could be breaking
>> someone's control flow).  Should qapi_enum_parse be taught to be
>> case-insensitive?  Or perhaps we answer related questions first: Do we
>> have any QAPI enums that have values differing only in case? Do we
>> prevent such QAPI definitions, to give us the potential of making the
>> parsing insensitive?
> 
> Case-sensitive everywhere is fine.  Case-insensitive everywhere also
> fine, just not my personal preference.  What's not fine is "guess
> whether this part of the interface is case-sensitive or not".
> 
> QMP is case-sensitive.  Let's keep it that way.
> 
> The -watchdog-action option has a case-insensitive argument.  The
> obvious way to remain misfeature-^Wbackwards compatible is converting
> the argument to lower case before handing it off to qapi_enum_parse.  I
> doubt it matters, but just doing it is less work than debating how far
> exactly we want to bend over backwards.
> 
> g_ascii_strdown() should do.  It only converts ASCII characters, but
> anything else is going to fail in qapi_enum_parse() anyway.
> 

On the other hand, the documentation enumerates the accepted values in
lowercase. So one can argue that upper- or mixed-case is just a misuse
of a bug in the code. But getting the code in is more important to me so
I'll do the strdown() conversion and sent yet another version.

Michal