[PATCH] lib: Add COMPILE_REGEXP macro to hide regexp constructors/destructors.
[NOTE: this is not the complete patch. Once ACKed, I will make the same mechanical change to all the other places in the library that use this pattern.] --- src/guestfs-internal.h | 24 ++++++++ src/inspect-fs-unix.c | 164 +++++++++++-------------------------------------- 2 files changed, 59 insertions(+), 129 deletions(-) diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index fd0c4a1..33d28f5 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -667,6 +667,30 @@ extern int guestfs___match6 (guestfs_h *g, const char *str, const pcre *re, char #define match4 guestfs___match4 #define match6 guestfs___match6 +/* Macro which compiles the regexp once when the library is loaded, + * and frees it when the library is unloaded. + */ +#define COMPILE_REGEXP(name,pattern,options) \ + static void compile_regexp_##name (void) __attribute__((constructor)); \ + static void free_regexp_##name (void) __attribute__((destructor)); \ + static pcre *name; \ + static void \ + compile_regexp_##name (void) \ + { \ + const char *err; \ + int offset; \ + name = pcre_compile ((pattern), (options), &err, &offset, NULL); \ + if (name == NULL) { \ + ignore_value (write (2, err, strlen (err))); \ + abort (); \ + } \ + } \ + static void \ + free_regexp_##name (void) \ + { \ + pcre_free (name); \ + } + /* stringsbuf.c */ struct stringsbuf { char **argv; diff --git a/src/inspect-fs-unix.c b/src/inspect-fs-unix.c index fb5cc1a..01a59f1 100644 --- a/src/inspect-fs-unix.c +++ b/src/inspect-fs-unix.c @@ -45,135 +45,41 @@ #include "guestfs-internal-actions.h" #include "guestfs_protocol.h" -/* Compile all the regular expressions once when the shared library is - * loaded. PCRE is thread safe so we're supposedly OK here if - * multiple threads call into the libguestfs API functions below - * simultaneously. - */ -static pcre *re_fedora; -static pcre *re_rhel_old; -static pcre *re_rhel; -static pcre *re_rhel_no_minor; -static pcre *re_centos_old; -static pcre *re_centos; -static pcre *re_centos_no_minor; -static pcre *re_scientific_linux_old; -static pcre *re_scientific_linux; -static pcre *re_scientific_linux_no_minor; -static pcre *re_oracle_linux_old; -static pcre *re_oracle_linux; -static pcre *re_oracle_linux_no_minor; -static pcre *re_major_minor; -static pcre *re_xdev; -static pcre *re_cciss; -static pcre *re_mdN; -static pcre *re_freebsd_mbr; -static pcre *re_freebsd_gpt; -static pcre *re_diskbyid; -static pcre *re_netbsd; -static pcre *re_opensuse; -static pcre *re_sles; -static pcre *re_nld; -static pcre *re_opensuse_version; -static pcre *re_sles_version; -static pcre *re_sles_patchlevel; -static pcre *re_minix; -static pcre *re_hurd_dev; - -static void compile_regexps (void) __attribute__((constructor)); -static void free_regexps (void) __attribute__((destructor)); - -static void -compile_regexps (void) -{ - const char *err; - int offset; - -#define COMPILE(re,pattern,options) \ - do { \ - re = pcre_compile ((pattern), (options), &err, &offset, NULL); \ - if (re == NULL) { \ - ignore_value (write (2, err, strlen (err))); \ - abort (); \ - } \ - } while (0) - - COMPILE (re_fedora, "Fedora release (\\d+)", 0); - COMPILE (re_rhel_old, - "Red Hat.*release (\\d+).*Update (\\d+)", 0); - COMPILE (re_rhel, - "Red Hat.*release (\\d+)\\.(\\d+)", 0); - COMPILE (re_rhel_no_minor, - "Red Hat.*release (\\d+)", 0); - COMPILE (re_centos_old, - "CentOS.*release (\\d+).*Update (\\d+)", 0); - COMPILE (re_centos, - "CentOS.*release (\\d+)\\.(\\d+)", 0); - COMPILE (re_centos_no_minor, - "CentOS.*release (\\d+)", 0); - COMPILE (re_scientific_linux_old, - "Scientific Linux.*release (\\d+).*Update (\\d+)", 0); - COMPILE (re_scientific_linux, - "Scientific Linux.*release (\\d+)\\.(\\d+)", 0); - COMPILE (re_scientific_linux_no_minor, - "Scientific Linux.*release (\\d+)", 0); - COMPILE (re_oracle_linux_old, - "Oracle Linux.*release (\\d+).*Update (\\d+)", 0); - COMPILE (re_oracle_linux, - "Oracle Linux.*release (\\d+)\\.(\\d+)", 0); - COMPILE (re_oracle_linux_no_minor, - "Oracle Linux.*release (\\d+)", 0); - COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0); - COMPILE (re_xdev, "^/dev/(h|s|v|xv)d([a-z]+)(\\d*)$", 0); - COMPILE (re_cciss, "^/dev/(cciss/c\\d+d\\d+)(?:p(\\d+))?$", 0); - COMPILE (re_mdN, "^(/dev/md\\d+)$", 0); - COMPILE (re_freebsd_mbr, "^/dev/(ada{0,1}|vtbd)(\\d+)s(\\d+)([a-z])$", 0); - COMPILE (re_freebsd_gpt, "^/dev/(ada{0,1}|vtbd)(\\d+)p(\\d+)$", 0); - COMPILE (re_diskbyid, "^/dev/disk/by-id/.*-part(\\d+)$", 0); - COMPILE (re_netbsd, "^NetBSD (\\d+)\\.(\\d+)", 0); - COMPILE (re_opensuse, "^(openSUSE|SuSE Linux|SUSE LINUX) ", 0); - COMPILE (re_sles, "^SUSE (Linux|LINUX) Enterprise ", 0); - COMPILE (re_nld, "^Novell Linux Desktop ", 0); - COMPILE (re_opensuse_version, "^VERSION = (\\d+)\\.(\\d+)", 0); - COMPILE (re_sles_version, "^VERSION = (\\d+)", 0); - COMPILE (re_sles_patchlevel, "^PATCHLEVEL = (\\d+)", 0); - COMPILE (re_minix, "^(\\d+)\\.(\\d+)(\\.(\\d+))?", 0); - COMPILE (re_hurd_dev, "^/dev/(h)d(\\d+)s(\\d+)$", 0); -} - -static void -free_regexps (void) -{ - pcre_free (re_fedora); - pcre_free (re_rhel_old); - pcre_free (re_rhel); - pcre_free (re_rhel_no_minor); - pcre_free (re_centos_old); - pcre_free (re_centos); - pcre_free (re_centos_no_minor); - pcre_free (re_scientific_linux_old); - pcre_free (re_scientific_linux); - pcre_free (re_scientific_linux_no_minor); - pcre_free (re_oracle_linux_old); - pcre_free (re_oracle_linux); - pcre_free (re_oracle_linux_no_minor); - pcre_free (re_major_minor); - pcre_free (re_xdev); - pcre_free (re_cciss); - pcre_free (re_mdN); - pcre_free (re_freebsd_mbr); - pcre_free (re_freebsd_gpt); - pcre_free (re_diskbyid); - pcre_free (re_netbsd); - pcre_free (re_opensuse); - pcre_free (re_sles); - pcre_free (re_nld); - pcre_free (re_opensuse_version); - pcre_free (re_sles_version); - pcre_free (re_sles_patchlevel); - pcre_free (re_minix); - pcre_free (re_hurd_dev); -} +COMPILE_REGEXP (re_fedora, "Fedora release (\\d+)", 0) +COMPILE_REGEXP (re_rhel_old, "Red Hat.*release (\\d+).*Update (\\d+)", 0) +COMPILE_REGEXP (re_rhel, "Red Hat.*release (\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_rhel_no_minor, "Red Hat.*release (\\d+)", 0) +COMPILE_REGEXP (re_centos_old, "CentOS.*release (\\d+).*Update (\\d+)", 0) +COMPILE_REGEXP (re_centos, "CentOS.*release (\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_centos_no_minor, "CentOS.*release (\\d+)", 0) +COMPILE_REGEXP (re_scientific_linux_old, + "Scientific Linux.*release (\\d+).*Update (\\d+)", 0) +COMPILE_REGEXP (re_scientific_linux, + "Scientific Linux.*release (\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_scientific_linux_no_minor, + "Scientific Linux.*release (\\d+)", 0) + COMPILE_REGEXP (re_oracle_linux_old, + "Oracle Linux.*release (\\d+).*Update (\\d+)", 0) +COMPILE_REGEXP (re_oracle_linux, + "Oracle Linux.*release (\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_oracle_linux_no_minor, "Oracle Linux.*release (\\d+)", 0) +COMPILE_REGEXP (re_major_minor, "(\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_xdev, "^/dev/(h|s|v|xv)d([a-z]+)(\\d*)$", 0) +COMPILE_REGEXP (re_cciss, "^/dev/(cciss/c\\d+d\\d+)(?:p(\\d+))?$", 0) +COMPILE_REGEXP (re_mdN, "^(/dev/md\\d+)$", 0) +COMPILE_REGEXP (re_freebsd_mbr, + "^/dev/(ada{0,1}|vtbd)(\\d+)s(\\d+)([a-z])$", 0) +COMPILE_REGEXP (re_freebsd_gpt, "^/dev/(ada{0,1}|vtbd)(\\d+)p(\\d+)$", 0) +COMPILE_REGEXP (re_diskbyid, "^/dev/disk/by-id/.*-part(\\d+)$", 0) +COMPILE_REGEXP (re_netbsd, "^NetBSD (\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_opensuse, "^(openSUSE|SuSE Linux|SUSE LINUX) ", 0) +COMPILE_REGEXP (re_sles, "^SUSE (Linux|LINUX) Enterprise ", 0) +COMPILE_REGEXP (re_nld, "^Novell Linux Desktop ", 0) +COMPILE_REGEXP (re_opensuse_version, "^VERSION = (\\d+)\\.(\\d+)", 0) +COMPILE_REGEXP (re_sles_version, "^VERSION = (\\d+)", 0) +COMPILE_REGEXP (re_sles_patchlevel, "^PATCHLEVEL = (\\d+)", 0) +COMPILE_REGEXP (re_minix, "^(\\d+)\\.(\\d+)(\\.(\\d+))?", 0) +COMPILE_REGEXP (re_hurd_dev, "^/dev/(h)d(\\d+)s(\\d+)$", 0) static void check_architecture (guestfs_h *g, struct inspect_fs *fs); static int check_hostname_unix (guestfs_h *g, struct inspect_fs *fs); -- 2.1.0
On Friday 28 November 2014 14:44:30 Richard W.M. Jones wrote:
[NOTE: this is not the complete patch. Once ACKed, I will make the same mechanical change to all the other places in the library that use this pattern.] --- src/guestfs-internal.h | 24 ++++++++ src/inspect-fs-unix.c | 164 +++++++++++-------------------------------------- 2 files changed, 59 insertions(+), 129 deletions(-)
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h index fd0c4a1..33d28f5 100644 --- a/src/guestfs-internal.h +++ b/src/guestfs-internal.h @@ -667,6 +667,30 @@ extern int guestfs___match6 (guestfs_h *g, const char *str, const pcre *re, char #define match4 guestfs___match4 #define match6 guestfs___match6
+/* Macro which compiles the regexp once when the library is loaded, + * and frees it when the library is unloaded. + */ +#define COMPILE_REGEXP(name,pattern,options) \ + static void compile_regexp_##name (void) __attribute__((constructor)); \ + static void free_regexp_##name (void) __attribute__((destructor)); \ + static pcre *name; \ + static void \ + compile_regexp_##name (void) \ + { \ + const char *err; \ + int offset; \ + name = pcre_compile ((pattern), (options), &err, &offset, NULL); \ + if (name == NULL) { \ + ignore_value (write (2, err, strlen (err))); \ + abort (); \ + } \ + } \ + static void \ + free_regexp_##name (void) \ + { \ + pcre_free (name); \ + }
Sounds okay, I guess it shouldn't introduce much overhead in the library loading? I also wonder whether we can delay the creation of those regexps (i.e. not just the ones mentioned in this patch) when just needed; something like the (untested): #define COMPILE_REGEXP(name,pattern,options) \ static void free_regexp_##name (void) __attribute__((destructor)); \ static pcre *_internal_##name; \ static void \ ##name (void) \ { \ if (_internal_##name == NULL) { \ const char *err; \ int offset; \ _internal_##name = pcre_compile ((pattern), (options), \ &err, &offset, NULL); \ if (_internal_##name == NULL) { \ ignore_value (write (2, err, strlen (err))); \ abort (); \ } \ } \ return _internal_##name; \ } \ static void \ free_regexp_##name (void) \ { \ if (_internal_##name) \ pcre_free (_internal_##name); \ } using it e.g.: match (re_major_minor (), ...) The only issue I could see is that the above is not thread-safe. -- Pino Toscano
On Fri, Nov 28, 2014 at 06:15:02PM +0100, Pino Toscano wrote:
Sounds okay, I guess it shouldn't introduce much overhead in the library loading?
I also wonder whether we can delay the creation of those regexps (i.e. not just the ones mentioned in this patch) when just needed; something like the (untested):
#define COMPILE_REGEXP(name,pattern,options) \ static void free_regexp_##name (void) __attribute__((destructor)); \ static pcre *_internal_##name; \ static void \ ##name (void) \ { \ if (_internal_##name == NULL) { \ const char *err; \ int offset; \ _internal_##name = pcre_compile ((pattern), (options), \ &err, &offset, NULL); \ if (_internal_##name == NULL) { \ ignore_value (write (2, err, strlen (err))); \ abort (); \ } \ } \ return _internal_##name; \ } \ static void \ free_regexp_##name (void) \ { \ if (_internal_##name) \ pcre_free (_internal_##name); \ }
using it e.g.: match (re_major_minor (), ...)
The only issue I could see is that the above is not thread-safe.
Yeah, thread safety is a problem. I wonder if ELF weak symbols can help? However AFAIK they only apply to functions. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (2)
-
Pino Toscano -
Richard W.M. Jones