Richard W.M. Jones
2014-Nov-28 14:44 UTC
[Libguestfs] [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
Pino Toscano
2014-Nov-28 17:15 UTC
Re: [Libguestfs] [PATCH] lib: Add COMPILE_REGEXP macro to hide regexp constructors/destructors.
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
Richard W.M. Jones
2014-Nov-28 19:58 UTC
Re: [Libguestfs] [PATCH] lib: Add COMPILE_REGEXP macro to hide regexp constructors/destructors.
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
Possibly Parallel Threads
- Re: [PATCH] lib: Add COMPILE_REGEXP macro to hide regexp constructors/destructors.
- [PATCH 1/2] lib: Allow the COMPILE_REGEXP macro to be used everywhere.
- [PATCH 2/2] inspect: switch to version struct for os major/minor version
- [PATCH] Update SuSE Linux detection.
- [PATCH stable-1.24] Fix fstab block device resolution for FreeBSD