virkeyfile: fix compilation error with clang

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

Posted by Pavel Hrdina 4 years, 4 months ago

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@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)
 #define CUR (*ctxt->cur)
 #define NEXT if (!IS_EOF) ctxt->cur++;
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Christophe de Dinechin 4 years, 4 months ago


> On 10 Dec 2019, at 15:11, Pavel Hrdina <phrdina@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@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.


> #define CUR (*ctxt->cur)
> #define NEXT if (!IS_EOF) ctxt->cur++;
> 
> -- 
> 2.23.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Pavel Hrdina 4 years, 4 months ago

On Tue, Dec 10, 2019 at 03:14:40PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 10 Dec 2019, at 15:11, Pavel Hrdina <phrdina@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@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.

> 
> 
> > #define CUR (*ctxt->cur)
> > #define NEXT if (!IS_EOF) ctxt->cur++;
> > 
> > -- 
> > 2.23.0
> > 
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
> 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Ján Tomko 4 years, 4 months ago

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@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@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@redhat.com>


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'

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Pavel Hrdina 4 years, 4 months ago

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@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@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@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>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrangé 4 years, 4 months ago

On Tue, Dec 10, 2019 at 04:03:56PM +0100, 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@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@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@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].

Yes, if GKeyFile is compatible with what we're currently parsing,
we should definitely just replace it

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Cole Robinson 4 years, 4 months ago

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list