[libvirt] [PATCH] virkeyfile: fix compilation error with clang

[Cole Robinson]

On 12/10/19 10:03 AM, Pavel Hrdina wrote:
> On Tue, Dec 10, 2019 at 03:57:59PM +0100, Ján Tomko wrote:
>> On Tue, Dec 10, 2019 at 03:29:43PM +0100, Pavel Hrdina wrote:
>>> On Tue, Dec 10, 2019 at 03:14:40PM +0100, Christophe de Dinechin wrote:
>>>>
>>>>
>>>>> On 10 Dec 2019, at 15:11, Pavel Hrdina <phrdina at redhat.com> wrote:
>>>>>
>>>>> Clang complains about condition being always true:
>>>>>
>>>>> src/util/virkeyfile.c:113:23: error: result of comparison of constant 128 with expression of type 'const char' is always true [-Werror,-Wtautological-constant-out-of-range-compare]
>>>>>    while (!IS_EOF && IS_ASCII(CUR) && CUR != ']')
>>>>>                      ^~~~~~~~~~~~~
>>>>> src/util/virkeyfile.c:80:26: note: expanded from macro 'IS_ASCII'
>>>>>                     ~~~ ^ ~~~
>>>>>
>>>>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>>>>> ---
>>>>> src/util/virkeyfile.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c
>>>>> index 816bfae96d..ee29bd7aa6 100644
>>>>> --- a/src/util/virkeyfile.c
>>>>> +++ b/src/util/virkeyfile.c
>>>>> @@ -77,7 +77,7 @@ struct _virKeyFileParserCtxt {
>>>>> #define IS_EOF (ctxt->cur >= ctxt->end)
>>>>> #define IS_EOL(c) (((c) == '\n') || ((c) == '\r'))
>>>>> #define IS_BLANK(c) (((c) == ' ') || ((c) == '\t'))
>>>>> -#define IS_ASCII(c) ((c) < 128)
>>>>> +#define IS_ASCII(c) (((unsigned char) c) < 128)
>>>>
>>>> Probably want parentheses around c.
>>>
>>> Right, I'll fix it before pushing.
>>>
>>
>> with that change:
>> Reviewed-by: Ján Tomko <jtomko at redhat.com>
> 
> Thanks, pushed now.
> 
>> But the pre-existingcode seems odd - this is used to parse the INI-like syntax
>> files for auth: https://libvirt.org/auth.html
>>
>> It does not really make sense to allow form feed or del in the group
>> entry name. I guess the only remotely reasonable character we'd block
>> by switching to g_ascii_isprint here is '\t'
> 
> I was considering to "fix" the code as well but when looking into what
> GLib offers we can replace this whole file with GKeyFile [1].
> 
> Pavel
> 
> [1] <https://developer.gnome.org/glib/stable/glib-Key-value-file-parser.html>

Ah cool. It would be nice to track a list somewhere of glib things that
can replace internal libvirt infrastructure. Maybe a subsection of the
bitesizedtasks wiki even though some of them will be not so bitesized.

What are the other glib bits people know about?

- Cole