| Message ID | 20190204211204.27321-3-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cutils: Cleanup, improve documentation | expand |
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)) > + > /** > * pstrcpy: > * @buf: buffer to copy string into With that added, Reviewed-by: Cornelia Huck <cohuck@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
diff --git a/hw/core/bus.c b/hw/core/bus.c index 4651f24486..dceb144075 100644 --- a/hw/core/bus.c +++ b/hw/core/bus.c @@ -18,7 +18,7 @@ */ #include "qemu/osdep.h" -#include "qemu-common.h" +#include "qemu/cutils.h" #include "hw/qdev.h" #include "qapi/error.h" diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 5da1439a8b..f36006bfce 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -1,4 +1,5 @@ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "net/net.h" #include "hw/qdev.h" #include "qapi/error.h" diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 811fdf913d..3ef42dfaf9 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -11,6 +11,7 @@ */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "qapi/error.h" #include "cpu.h" #include "hw/boards.h" diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c index 7237b4162e..86f65fd474 100644 --- a/hw/scsi/scsi-generic.c +++ b/hw/scsi/scsi-generic.c @@ -12,8 +12,8 @@ */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "qapi/error.h" -#include "qemu-common.h" #include "qemu/error-report.h" #include "hw/scsi/scsi.h" #include "hw/scsi/emulation.h" diff --git a/include/qemu-common.h b/include/qemu-common.h index 760527294f..ed43ae286d 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -33,22 +33,6 @@ int qemu_main(int argc, char **argv, char **envp); void qemu_get_timedate(struct tm *tm, int offset); int qemu_timedate_diff(struct tm *tm); -#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)) - void *qemu_oom_check(void *ptr); ssize_t qemu_write_full(int fd, const void *buf, size_t count) 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. + */ +#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)) + /** * pstrcpy: * @buf: buffer to copy string into diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c index e9b266bb70..ea93ae05d9 100644 --- a/qapi/qapi-util.c +++ b/qapi/qapi-util.c @@ -11,8 +11,8 @@ */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "qapi/error.h" -#include "qemu-common.h" const char *qapi_enum_lookup(const QEnumLookup *lookup, int val) { diff --git a/qobject/json-parser.c b/qobject/json-parser.c index d8eb210c0c..cad019d4cc 100644 --- a/qobject/json-parser.c +++ b/qobject/json-parser.c @@ -15,7 +15,6 @@ #include "qemu/cutils.h" #include "qemu/unicode.h" #include "qapi/error.h" -#include "qemu-common.h" #include "qapi/qmp/qbool.h" #include "qapi/qmp/qdict.h" #include "qapi/qmp/qlist.h" diff --git a/target/ppc/monitor.c b/target/ppc/monitor.c index 04deec8030..173177081b 100644 --- a/target/ppc/monitor.c +++ b/target/ppc/monitor.c @@ -22,6 +22,7 @@ * THE SOFTWARE. */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "cpu.h" #include "monitor/monitor.h" #include "monitor/hmp-target.h" diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 28d7e5302f..77fb2372b9 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -19,6 +19,7 @@ #include "qemu/osdep.h" #include "qemu/log.h" +#include "qemu/cutils.h" #include "cpu.h" #include "exec/exec-all.h" #include "qapi/error.h" diff --git a/ui/keymaps.c b/ui/keymaps.c index 6e44f738ed..a672fe3dd3 100644 --- a/ui/keymaps.c +++ b/ui/keymaps.c @@ -23,6 +23,7 @@ */ #include "qemu/osdep.h" +#include "qemu/cutils.h" #include "keymaps.h" #include "sysemu/sysemu.h" #include "trace.h" diff --git a/util/id.c b/util/id.c index 6141352955..ca21a77522 100644 --- a/util/id.c +++ b/util/id.c @@ -11,7 +11,7 @@ */ #include "qemu/osdep.h" -#include "qemu-common.h" +#include "qemu/cutils.h" #include "qemu/id.h" bool id_wellformed(const char *id) diff --git a/util/readline.c b/util/readline.c index ec91ee0fea..f3d8b0698a 100644 --- a/util/readline.c +++ b/util/readline.c @@ -23,7 +23,6 @@ */ #include "qemu/osdep.h" -#include "qemu-common.h" #include "qemu/readline.h" #include "qemu/cutils.h"