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