[libvirt] [PATCH] maint: avoid locale-sensitivity in string case comparisons
strcase{cmp/str} have the drawback of being sensitive to the global locale; this is unacceptable in a library setting. Prefer a hard-coded C locale alternative for all but virsh, which is user facing and where the global locale isn't changing externally. * .gnulib: Update to latest, for c-strcasestr change. * bootstrap.conf (gnulib_modules): Drop strcasestr, add c-strcase and c-strcasestr. * cfg.mk (sc_avoid_strcase): New rule. (exclude_file_name_regexp--sc_avoid_strcase): New exception. * src/internal.h (STRCASEEQ, STRCASENEQ, STRCASEEQLEN) (STRCASENEQLEN): Adjust offenders. * src/qemu/qemu_monitor_text.c (qemuMonitorTextEjectMedia): Likewise. * tools/virsh.c (namesorter): Document exception. --- Inspired by today's earlier patch that started using strcasester. This also goes along with our policy of no ctype, just c-ctype. .gnulib | 2 +- bootstrap.conf | 3 ++- cfg.mk | 7 +++++++ src/internal.h | 10 ++++++---- src/qemu/qemu_monitor_text.c | 3 ++- tools/virsh.c | 1 + 6 files changed, 19 insertions(+), 7 deletions(-) diff --git a/.gnulib b/.gnulib index 422ab2e..790645d 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 422ab2e0d70ed348e2fd0a82558be38e5859011a +Subproject commit 790645d837f8084991421107fba639b110d58335 diff --git a/bootstrap.conf b/bootstrap.conf index 6e10828..733c354 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -22,6 +22,8 @@ gnulib_modules=' areadlink base64 c-ctype +c-strcase +c-strcasestr canonicalize-lgpl chown close @@ -63,7 +65,6 @@ sigpipe snprintf socket stpcpy -strcasestr strchrnul strndup strerror diff --git a/cfg.mk b/cfg.mk index ac419f7..f802cee 100644 --- a/cfg.mk +++ b/cfg.mk @@ -349,6 +349,11 @@ sc_avoid_ctype_macros: halt="don't use ctype macros (use c-ctype.h)" \ $(_sc_search_regexp) +sc_avoid_strcase: + @prohibit='\bstrn?case(cmp|str) *\(' \ + halt="don't use raw strcase functions (use c-strcase instead)" \ + $(_sc_search_regexp) + sc_prohibit_virBufferAdd_with_string_literal: @prohibit='\<virBufferAdd *\([^,]+, *"[^"]' \ halt='use virBufferAddLit, not virBufferAdd, with a string literal' \ @@ -547,6 +552,8 @@ _makefile_at_at_check_exceptions = ' && !/(SCHEMA|SYSCONF)DIR/' syntax-check: $(top_srcdir)/HACKING # List all syntax-check exemptions: +exclude_file_name_regexp--sc_avoid_strcase = ^tools/virsh\.c$$ + _src1=libvirt|fdstream|qemu/qemu_monitor|util/(command|util)|xen/xend_internal exclude_file_name_regexp--sc_avoid_write = \ ^(src/($(_src1))|daemon/libvirtd|tools/console)\.c$$ diff --git a/src/internal.h b/src/internal.h index be97801..2afbd8d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -44,6 +44,8 @@ # include "libvirt_internal.h" +# include "c-strcase.h" + /* On architectures which lack these limits, define them (ie. Cygwin). * Note that the libvirt code should be robust enough to handle the * case where actual value is longer than these limits (eg. by setting @@ -64,13 +66,13 @@ /* String equality tests, suggested by Jim Meyering. */ # define STREQ(a,b) (strcmp(a,b) == 0) -# define STRCASEEQ(a,b) (strcasecmp(a,b) == 0) +# define STRCASEEQ(a,b) (c_strcasecmp(a,b) == 0) # define STRNEQ(a,b) (strcmp(a,b) != 0) -# define STRCASENEQ(a,b) (strcasecmp(a,b) != 0) +# define STRCASENEQ(a,b) (c_strcasecmp(a,b) != 0) # define STREQLEN(a,b,n) (strncmp(a,b,n) == 0) -# define STRCASEEQLEN(a,b,n) (strncasecmp(a,b,n) == 0) +# define STRCASEEQLEN(a,b,n) (c_strncasecmp(a,b,n) == 0) # define STRNEQLEN(a,b,n) (strncmp(a,b,n) != 0) -# define STRCASENEQLEN(a,b,n) (strncasecmp(a,b,n) != 0) +# define STRCASENEQLEN(a,b,n) (c_strncasecmp(a,b,n) != 0) # define STRPREFIX(a,b) (strncmp(a,b,strlen(b)) == 0) # define STRSKIP(a,b) (STRPREFIX(a,b) ? (a) + strlen(b) : NULL) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index e0e3292..168c60f 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -33,6 +33,7 @@ #include "qemu_monitor_text.h" #include "qemu_command.h" #include "c-ctype.h" +#include "c-strcasestr.h" #include "memory.h" #include "logging.h" #include "driver.h" @@ -934,7 +935,7 @@ int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, /* If the command failed qemu prints: * device not found, device is locked ... * No message is printed on success it seems */ - if (strcasestr(reply, "device ")) { + if (c_strcasestr(reply, "device ")) { qemuReportError(VIR_ERR_OPERATION_FAILED, _("could not eject media on %s: %s"), devname, reply); goto cleanup; diff --git a/tools/virsh.c b/tools/virsh.c index faeaf47..3c759b9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -432,6 +432,7 @@ static int namesorter(const void *a, const void *b) { const char **sa = (const char**)a; const char **sb = (const char**)b; + /* User visible sort, so we want locale-specific case comparison. */ return strcasecmp(*sa, *sb); } -- 1.7.4
On Wed, Mar 30, 2011 at 08:29:53PM -0600, Eric Blake wrote:
strcase{cmp/str} have the drawback of being sensitive to the global locale; this is unacceptable in a library setting. Prefer a hard-coded C locale alternative for all but virsh, which is user facing and where the global locale isn't changing externally.
* .gnulib: Update to latest, for c-strcasestr change. * bootstrap.conf (gnulib_modules): Drop strcasestr, add c-strcase and c-strcasestr. * cfg.mk (sc_avoid_strcase): New rule. (exclude_file_name_regexp--sc_avoid_strcase): New exception. * src/internal.h (STRCASEEQ, STRCASENEQ, STRCASEEQLEN) (STRCASENEQLEN): Adjust offenders. * src/qemu/qemu_monitor_text.c (qemuMonitorTextEjectMedia): Likewise. * tools/virsh.c (namesorter): Document exception. ---
Inspired by today's earlier patch that started using strcasester. This also goes along with our policy of no ctype, just c-ctype.
.gnulib | 2 +- bootstrap.conf | 3 ++- cfg.mk | 7 +++++++ src/internal.h | 10 ++++++---- src/qemu/qemu_monitor_text.c | 3 ++- tools/virsh.c | 1 + 6 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/.gnulib b/.gnulib index 422ab2e..790645d 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 422ab2e0d70ed348e2fd0a82558be38e5859011a +Subproject commit 790645d837f8084991421107fba639b110d58335 diff --git a/bootstrap.conf b/bootstrap.conf index 6e10828..733c354 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -22,6 +22,8 @@ gnulib_modules=' areadlink base64 c-ctype +c-strcase +c-strcasestr canonicalize-lgpl chown close @@ -63,7 +65,6 @@ sigpipe snprintf socket stpcpy -strcasestr strchrnul strndup strerror diff --git a/cfg.mk b/cfg.mk index ac419f7..f802cee 100644 --- a/cfg.mk +++ b/cfg.mk @@ -349,6 +349,11 @@ sc_avoid_ctype_macros: halt="don't use ctype macros (use c-ctype.h)" \ $(_sc_search_regexp)
+sc_avoid_strcase: + @prohibit='\bstrn?case(cmp|str) *\(' \ + halt="don't use raw strcase functions (use c-strcase instead)" \ + $(_sc_search_regexp) + sc_prohibit_virBufferAdd_with_string_literal: @prohibit='\<virBufferAdd *\([^,]+, *"[^"]' \ halt='use virBufferAddLit, not virBufferAdd, with a string literal' \ @@ -547,6 +552,8 @@ _makefile_at_at_check_exceptions = ' && !/(SCHEMA|SYSCONF)DIR/' syntax-check: $(top_srcdir)/HACKING
# List all syntax-check exemptions: +exclude_file_name_regexp--sc_avoid_strcase = ^tools/virsh\.c$$ + _src1=libvirt|fdstream|qemu/qemu_monitor|util/(command|util)|xen/xend_internal exclude_file_name_regexp--sc_avoid_write = \ ^(src/($(_src1))|daemon/libvirtd|tools/console)\.c$$ diff --git a/src/internal.h b/src/internal.h index be97801..2afbd8d 100644 --- a/src/internal.h +++ b/src/internal.h @@ -44,6 +44,8 @@
# include "libvirt_internal.h"
+# include "c-strcase.h" + /* On architectures which lack these limits, define them (ie. Cygwin). * Note that the libvirt code should be robust enough to handle the * case where actual value is longer than these limits (eg. by setting @@ -64,13 +66,13 @@
/* String equality tests, suggested by Jim Meyering. */ # define STREQ(a,b) (strcmp(a,b) == 0) -# define STRCASEEQ(a,b) (strcasecmp(a,b) == 0) +# define STRCASEEQ(a,b) (c_strcasecmp(a,b) == 0) # define STRNEQ(a,b) (strcmp(a,b) != 0) -# define STRCASENEQ(a,b) (strcasecmp(a,b) != 0) +# define STRCASENEQ(a,b) (c_strcasecmp(a,b) != 0) # define STREQLEN(a,b,n) (strncmp(a,b,n) == 0) -# define STRCASEEQLEN(a,b,n) (strncasecmp(a,b,n) == 0) +# define STRCASEEQLEN(a,b,n) (c_strncasecmp(a,b,n) == 0) # define STRNEQLEN(a,b,n) (strncmp(a,b,n) != 0) -# define STRCASENEQLEN(a,b,n) (strncasecmp(a,b,n) != 0) +# define STRCASENEQLEN(a,b,n) (c_strncasecmp(a,b,n) != 0) # define STRPREFIX(a,b) (strncmp(a,b,strlen(b)) == 0) # define STRSKIP(a,b) (STRPREFIX(a,b) ? (a) + strlen(b) : NULL)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index e0e3292..168c60f 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -33,6 +33,7 @@ #include "qemu_monitor_text.h" #include "qemu_command.h" #include "c-ctype.h" +#include "c-strcasestr.h" #include "memory.h" #include "logging.h" #include "driver.h" @@ -934,7 +935,7 @@ int qemuMonitorTextEjectMedia(qemuMonitorPtr mon, /* If the command failed qemu prints: * device not found, device is locked ... * No message is printed on success it seems */ - if (strcasestr(reply, "device ")) { + if (c_strcasestr(reply, "device ")) { qemuReportError(VIR_ERR_OPERATION_FAILED, _("could not eject media on %s: %s"), devname, reply); goto cleanup; diff --git a/tools/virsh.c b/tools/virsh.c index faeaf47..3c759b9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -432,6 +432,7 @@ static int namesorter(const void *a, const void *b) { const char **sa = (const char**)a; const char **sb = (const char**)b;
+ /* User visible sort, so we want locale-specific case comparison. */ return strcasecmp(*sa, *sb); }
ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 03/30/2011 10:44 PM, Daniel Veillard wrote:
On Wed, Mar 30, 2011 at 08:29:53PM -0600, Eric Blake wrote:
strcase{cmp/str} have the drawback of being sensitive to the global locale; this is unacceptable in a library setting. Prefer a hard-coded C locale alternative for all but virsh, which is user facing and where the global locale isn't changing externally.
* .gnulib: Update to latest, for c-strcasestr change.
+++ b/tools/virsh.c @@ -432,6 +432,7 @@ static int namesorter(const void *a, const void *b) { const char **sa = (const char**)a; const char **sb = (const char**)b;
+ /* User visible sort, so we want locale-specific case comparison. */ return strcasecmp(*sa, *sb);
Hmm, maybe strcoll would be better, but that has undefined behavior if not all strings have valid encodings. It's a perfect fit for gnulib's mbmemcasecoll module, except that it is GPL with no chance of being relaxed (that really is a value-added function that goes way beyond libc basics). And while virsh could link with GPL stuff, I'm not ready to deal with the hassle of two gnulib-tool invocations (lgpl for the library, gpl for virsh). So I'm fine with keeping strcasecmp for now.
ACK
Thanks; pushed. I've got one more gnulib update coming later today which should fix the non-blocking pipe fd problem for mingw. Oh, and I realized I forgot to list the gnulib commits being pulled in by this change: * .gnulib 422ab2e...790645d (47):
stdio: Avoid GCC >= 4.4 warnings when using %lld and similar on mingw. passfd: fix scoping bug passfd: standardize coding conventions passfd: fix incorrect sendmsg arguments c-strcasestr: Relicense under LGPLv2+. doc: update users.txt tests: readlink* ("",... fails with EINVAL on newer kernels Relicense some modules under LGPLv2+, for libidn2. lib-symbol-visibility: Add a notice. autoupdate getaddrinfo: Doc fix. getaddrinfo: Doc fix. unictype/property-byname: Reduce the number of load-time relocations. unictype/property-byname: Allow omitted word separators and aliases. unictype/joininggroup-byname: Allow hyphens, omitted word separators. unictype/joiningtype-byname: Recognize long names as well. Tests for module 'unictype/joiningtype-longname'. New module 'unictype/joiningtype-longname'. unictype/bidiclass-byname: Recognize long names as well. Tests for module 'unictype/bidiclass-longname'. New module 'unictype/bidiclass-longname'. unictype/bidi*: Rename modules, part 2. unictype/bidi*: Rename modules. unictype/bidi*: Rename functions, part 2. New module 'unictype/combining-class-all'. Tests for module 'unictype/combining-class-byname'. New module 'unictype/combining-class-byname'. Tests for module 'unictype/combining-class-longname'. New module 'unictype/combining-class-longname'. Tests for module 'unictype/combining-class-name'. New module 'unictype/combining-class-name'. unictype/combining-class: Rename source files. unictype: Update list of canonical combining classes. unictype/category-byname: Recognize long names as well. Tests for module 'unictype/category-longname'. New module 'unictype/category-longname'. New module 'unictype/category-LC', part 2. Tests for module 'unictype/category-LC'. New module 'unictype/category-LC'. xmalloc: revert yesterday's regression maint.mk: add missing version to VC-tag valgrind: do leak checking, and exit with code 1 on error (not 0) posix-modules: say what it does. xmalloc: Do not leak if underlying realloc is C99 compatible. realloc: document portability problem autoupdate doc: add cvsps and tmpwatch to users.txt
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel Veillard -
Eric Blake