Re: [Qemu-devel] [PATCH v3 2/3] util/cutils: Move ctype macros to "cutils.h"

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Feb 05, 2019 at 11:32:19AM +0100, Cornelia Huck wrote:
> On Mon,  4 Feb 2019 22:12:03 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > Introduced in cd390083ad1, these macros don't need to be in
> > a generic header.
> > Add documentation to justify their use.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > v2: Fixed checkpatch warnings (tabs)
> > ---
> >  hw/core/bus.c              |  2 +-
> >  hw/core/qdev-properties.c  |  1 +
> >  hw/s390x/s390-virtio-ccw.c |  1 +
> >  hw/scsi/scsi-generic.c     |  2 +-
> >  include/qemu-common.h      | 16 ----------------
> >  include/qemu/cutils.h      | 25 +++++++++++++++++++++++++
> >  qapi/qapi-util.c           |  2 +-
> >  qobject/json-parser.c      |  1 -
> >  target/ppc/monitor.c       |  1 +
> >  target/riscv/cpu.c         |  1 +
> >  ui/keymaps.c               |  1 +
> >  util/id.c                  |  2 +-
> >  util/readline.c            |  1 -
> >  13 files changed, 34 insertions(+), 22 deletions(-)
> 
> > diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> > index 9ee40470e3..644f2d75bd 100644
> > --- a/include/qemu/cutils.h
> > +++ b/include/qemu/cutils.h
> > @@ -3,6 +3,31 @@
> >  
> >  #include "qemu/fprintf-fn.h"
> >  
> > +/**
> > + * unsigned ctype macros:
> > + *
> > + * The standards require that the argument for these functions
> > + * is either EOF or a value that is representable in the type
> > + * unsigned char. If the argument is of type char, it must be
> > + * cast to unsigned char. This is what these macros do,
> > + * avoiding 'signed to unsigned' conversion warnings.
> 
> I'd add
> 
> "Note that these macros are intended for use with char arguments only,
> they cannot handle EOF."
> 
> > + */
> > +#define qemu_isalnum(c)     isalnum((unsigned char)(c))
> > +#define qemu_isalpha(c)     isalpha((unsigned char)(c))
> > +#define qemu_iscntrl(c)     iscntrl((unsigned char)(c))
> > +#define qemu_isdigit(c)     isdigit((unsigned char)(c))
> > +#define qemu_isgraph(c)     isgraph((unsigned char)(c))
> > +#define qemu_islower(c)     islower((unsigned char)(c))
> > +#define qemu_isprint(c)     isprint((unsigned char)(c))
> > +#define qemu_ispunct(c)     ispunct((unsigned char)(c))
> > +#define qemu_isspace(c)     isspace((unsigned char)(c))
> > +#define qemu_isupper(c)     isupper((unsigned char)(c))
> > +#define qemu_isxdigit(c)    isxdigit((unsigned char)(c))
> > +#define qemu_tolower(c)     tolower((unsigned char)(c))
> > +#define qemu_toupper(c)     toupper((unsigned char)(c))
> > +#define qemu_isascii(c)     isascii((unsigned char)(c))
> > +#define qemu_toascii(c)     toascii((unsigned char)(c))

Not a problem with your patch, since this is just code movement, but
I would note that some (many?) uses of these functions in QEMU are likely
wrong / bad. These functions all check according to the current locale
rules, while IME most developers assume these APIs are following ASCII
match rules. IOW, these may well be allowing more characters through
than expected.

We might want to consider whether some callers need switching to use
the glib provided APIs that explicitly match in ASCII rules:

  https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-ascii-isalnum

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