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

[Markus Armbruster]

Michal Privoznik <address@hidden> writes:

> 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.

I quite agree, but...

>                       But getting the code in is more important to me so
> I'll do the strdown() conversion and sent yet another version.

... like you, I have to pick my battles.  A respin adding the stupid
case conversion seems safer for both of us than risking a debate on how
far we need to go for backward compatibility.