[libvirt] [PATCH 00/12] Use GRegex instead of regcomp (glib chronicles)
This removes the need to use gnulib's regex module. Ján Tomko (12): libxl: do not use G_REGEX_EXTENDED remove unused regex.h includes libxl: use GRegex in libxlGetAutoballoonConf libxl: use g_autofree in xenParseSxprVifRate libxl: use GRegex in xenParseSxprVifRate libxl: remove 'ret' from xenParseSxprVifRate storage: use GRegex virStorageBackendLogicalParseVolExtents util: use GRegex in virCommandRunRegex util: use GRegex for virLogRegex util: use GRegex in virStringSearch util: use GRegex in virStringMatch tests: use GRegex in vboxsnapshotxmltest src/libxl/libxl_capabilities.c | 2 +- src/libxl/libxl_conf.c | 20 ++++------ src/libxl/libxl_driver.c | 1 - src/libxl/xen_common.c | 33 ++++++----------- src/storage/storage_backend_logical.c | 49 +++++++------------------ src/storage/storage_util.c | 1 - src/util/vircommand.c | 37 +++++++------------ src/util/viriscsi.c | 2 - src/util/virlog.c | 15 +++----- src/util/virnetdevtap.c | 1 - src/util/virstring.c | 53 +++++++++++---------------- tests/testutils.c | 1 - tests/vboxsnapshotxmltest.c | 20 ++++------ 13 files changed, 80 insertions(+), 155 deletions(-) -- 2.21.0
This flag is not needed to use extended regular expression syntax with GRegex and it makes GRegex ignore whitespace in the regex. Remove the unintended usage, even though it should not matter in this case. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxl/libxl_capabilities.c b/src/libxl/libxl_capabilities.c index fe792e9a82..0fb9c0b344 100644 --- a/src/libxl/libxl_capabilities.c +++ b/src/libxl/libxl_capabilities.c @@ -397,7 +397,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) return -1; } - regex = g_regex_new(XEN_CAP_REGEX, G_REGEX_EXTENDED, 0, &err); + regex = g_regex_new(XEN_CAP_REGEX, 0, 0, &err); if (!regex) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to compile regex %s"), err->message); -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:42 +0100, Ján Tomko wrote:
This flag is not needed to use extended regular expression syntax with GRegex and it makes GRegex ignore whitespace in the regex.
Remove the unintended usage, even though it should not matter in this case.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The code using regexes got moved, but the include stayed. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_driver.c | 1 - src/storage/storage_util.c | 1 - src/util/viriscsi.c | 2 -- src/util/virnetdevtap.c | 1 - tests/testutils.c | 1 - 5 files changed, 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 277d99b2b0..ec6a010030 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -27,7 +27,6 @@ #include <libxl_utils.h> #include <xenstore.h> #include <fcntl.h> -#include <regex.h> #include "internal.h" #include "virlog.h" diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index f91c2c64ee..9ba9bb2a57 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -18,7 +18,6 @@ #include <config.h> -#include <regex.h> #include <sys/types.h> #include <sys/wait.h> #include <unistd.h> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 9f4c8f4e03..da27894b0b 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -22,8 +22,6 @@ #include <config.h> -#include <regex.h> - #include "viriscsi.h" #include "viralloc.h" diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index aea4c91a6a..445aaa2727 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -32,7 +32,6 @@ #include "datatypes.h" #include <unistd.h> -#include <regex.h> #include <sys/types.h> #include <sys/ioctl.h> #include <net/if.h> diff --git a/tests/testutils.c b/tests/testutils.c index a3bedd0fef..3cd3391dfa 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -25,7 +25,6 @@ #include <sys/types.h> #include <sys/stat.h> #include <sys/wait.h> -#include <regex.h> #include <unistd.h> #include <fcntl.h> #include "testutils.h" -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:43 +0100, Ján Tomko wrote:
The code using regexes got moved, but the include stayed.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_driver.c | 1 - src/storage/storage_util.c | 1 - src/util/viriscsi.c | 2 -- src/util/virnetdevtap.c | 1 - tests/testutils.c | 1 - 5 files changed, 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Replace the use of regcomp with GRegex. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_conf.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 920f228d6a..be7d85b264 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -22,7 +22,6 @@ #include <config.h> -#include <regex.h> #include <libxl.h> #include <sys/types.h> #include <sys/socket.h> @@ -1664,7 +1663,8 @@ static int libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, virConfPtr conf) { - regex_t regex; + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL; int res; res = virConfGetValueBool(conf, "autoballoon", &cfg->autoballoon); @@ -1673,21 +1673,15 @@ libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, else if (res == 1) return 0; - if ((res = regcomp(®ex, - "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )", - REG_NOSUB | REG_EXTENDED)) != 0) { - char error[100]; - regerror(res, ®ex, error, sizeof(error)); + regex = g_regex_new("(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )", + 0, 0, &err); + if (!regex) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), - error); - + _("Failed to compile regex %s"), err->message); return -1; } - res = regexec(®ex, cfg->verInfo->commandline, 0, NULL, 0); - regfree(®ex); - cfg->autoballoon = res == REG_NOMATCH; + cfg->autoballoon = !g_regex_match(regex, cfg->verInfo->commandline, 0, NULL); return 0; } -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:44 +0100, Ján Tomko wrote:
Replace the use of regcomp with GRegex.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/libxl_conf.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/xen_common.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index c31a5c952e..c06ab6e995 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1060,7 +1060,7 @@ static const char *vif_bytes_per_sec_re = "^[0-9]+[GMK]?[Bb]/s$"; static int xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) { - char *trate = NULL; + g_autofree char *trate = NULL; char *p; regex_t rec; int err; @@ -1109,7 +1109,6 @@ xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) cleanup: regfree(&rec); - VIR_FREE(trate); return ret; } -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:45 +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/xen_common.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Use GRegex from GLib instead of regcomp. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/xen_common.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index c06ab6e995..15a2db8add 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -24,8 +24,6 @@ #include <config.h> -#include <regex.h> - #include "internal.h" #include "virerror.h" #include "virconf.h" @@ -1060,10 +1058,10 @@ static const char *vif_bytes_per_sec_re = "^[0-9]+[GMK]?[Bb]/s$"; static int xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) { + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL; g_autofree char *trate = NULL; char *p; - regex_t rec; - int err; char *suffix; unsigned long long tmp; int ret = -1; @@ -1074,17 +1072,14 @@ xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) if (p != NULL) *p = 0; - err = regcomp(&rec, vif_bytes_per_sec_re, REG_EXTENDED|REG_NOSUB); - if (err != 0) { - char error[100]; - regerror(err, &rec, error, sizeof(error)); + regex = g_regex_new(vif_bytes_per_sec_re, 0, 0, &err); + if (!regex) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regular expression '%s': %s"), - vif_bytes_per_sec_re, error); - goto cleanup; + _("Failed to compile regex %s"), err->message); + return -1; } - if (regexec(&rec, trate, 0, NULL, 0)) { + if (!g_regex_match(regex, trate, 0, NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid rate '%s' specified"), rate); goto cleanup; @@ -1108,7 +1103,6 @@ xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) ret = 0; cleanup: - regfree(&rec); return ret; } -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:46 +0100, Ján Tomko wrote:
Use GRegex from GLib instead of regcomp.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/xen_common.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Now that the cleanup section is empty, the ret variable is no longer necessary. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/xen_common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c index 15a2db8add..0cb7f0f331 100644 --- a/src/libxl/xen_common.c +++ b/src/libxl/xen_common.c @@ -1064,7 +1064,6 @@ xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) char *p; char *suffix; unsigned long long tmp; - int ret = -1; trate = g_strdup(rate); @@ -1082,13 +1081,13 @@ xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) if (!g_regex_match(regex, trate, 0, NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid rate '%s' specified"), rate); - goto cleanup; + return -1; } if (virStrToLong_ull(rate, &suffix, 10, &tmp)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse rate '%s'"), rate); - goto cleanup; + return -1; } if (*suffix == 'G') @@ -1100,10 +1099,7 @@ xenParseSxprVifRate(const char *rate, unsigned long long *kbytes_per_sec) tmp /= 8; *kbytes_per_sec = tmp; - ret = 0; - - cleanup: - return ret; + return 0; } -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:47 +0100, Ján Tomko wrote:
Now that the cleanup section is empty, the ret variable is no longer necessary.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/libxl/xen_common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Using GRegex simplifies the code since g_match_info_fetch will copy the matched substring for us. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 49 +++++++-------------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 48023abd1a..d6c81e25b1 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -23,7 +23,6 @@ #include <sys/wait.h> #include <sys/stat.h> -#include <regex.h> #include <unistd.h> #include <fcntl.h> @@ -121,16 +120,16 @@ static int virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, char **const groups) { + g_autoptr(GRegex) re = NULL; + g_autoptr(GError) err = NULL; + g_autoptr(GMatchInfo) info = NULL; int nextents, ret = -1; const char *regex_unit = "(\\S+)\\((\\S+)\\)"; char *p = NULL; size_t i; - int err, nvars; unsigned long long offset, size, length; virStorageVolSourceExtent extent; g_autofree char *regex = NULL; - g_autofree regex_t *reg = NULL; - g_autofree regmatch_t *vars = NULL; memset(&extent, 0, sizeof(extent)); @@ -174,29 +173,14 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, strcat(regex, regex_unit); } - if (VIR_ALLOC(reg) < 0) - goto cleanup; - - /* Each extent has a "path:offset" pair, and vars[0] will - * be the whole matched string. - */ - nvars = (nextents * 2) + 1; - if (VIR_ALLOC_N(vars, nvars) < 0) - goto cleanup; - - err = regcomp(reg, regex, REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, reg, error, sizeof(error)); + re = g_regex_new(regex, 0, 0, &err); + if (!re) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), - error); - goto cleanup; + _("Failed to compile regex %s"), err->message); + return -1; } - err = regexec(reg, groups[3], nvars, vars, 0); - regfree(reg); - if (err != 0) { + if (!g_regex_match(re, groups[3], 0, &info)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent devices value")); goto cleanup; @@ -204,23 +188,16 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, p = groups[3]; - /* vars[0] is skipped */ + /* Each extent has a "path:offset" pair, and match #0 + * is the whole matched string. + */ for (i = 0; i < nextents; i++) { size_t j; - int len; g_autofree char *offset_str = NULL; j = (i * 2) + 1; - len = vars[j].rm_eo - vars[j].rm_so; - p[vars[j].rm_eo] = '\0'; - - if (VIR_STRNDUP(extent.path, - p + vars[j].rm_so, len) < 0) - goto cleanup; - - len = vars[j + 1].rm_eo - vars[j + 1].rm_so; - if (VIR_STRNDUP(offset_str, p + vars[j + 1].rm_so, len) < 0) - goto cleanup; + extent.path = g_match_info_fetch(info, j); + offset_str = g_match_info_fetch(info, j + 1); if (virStrToLong_ull(offset_str, NULL, 10, &offset) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:48 +0100, Ján Tomko wrote:
Using GRegex simplifies the code since g_match_info_fetch will copy the matched substring for us.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 49 +++++++-------------------- 1 file changed, 13 insertions(+), 36 deletions(-)
I'm getting a build failure with this patch: ../../src/storage/storage_backend_logical.c:128:11: error: variable 'p' set but not used [-Werror=unused-but-set-variable] 128 | char *p = NULL;
On Thu, Nov 14, 2019 at 08:11:46AM +0100, Peter Krempa wrote:
On Wed, Nov 13, 2019 at 16:48:48 +0100, Ján Tomko wrote:
Using GRegex simplifies the code since g_match_info_fetch will copy the matched substring for us.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 49 +++++++-------------------- 1 file changed, 13 insertions(+), 36 deletions(-)
I'm getting a build failure with this patch:
../../src/storage/storage_backend_logical.c:128:11: error: variable 'p' set but not used [-Werror=unused-but-set-variable] 128 | char *p = NULL;
Oops, another case where GCC reports a slightly different set of warnings than CLang. Consider the following squashed in. Jano diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index d6c81e25b1..42dec05ba0 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -125,7 +125,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, g_autoptr(GMatchInfo) info = NULL; int nextents, ret = -1; const char *regex_unit = "(\\S+)\\((\\S+)\\)"; - char *p = NULL; size_t i; unsigned long long offset, size, length; virStorageVolSourceExtent extent; @@ -186,8 +185,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, goto cleanup; } - p = groups[3]; - /* Each extent has a "path:offset" pair, and match #0 * is the whole matched string. */
On Thu, Nov 14, 2019 at 13:14:33 +0100, Ján Tomko wrote:
On Thu, Nov 14, 2019 at 08:11:46AM +0100, Peter Krempa wrote:
On Wed, Nov 13, 2019 at 16:48:48 +0100, Ján Tomko wrote:
Using GRegex simplifies the code since g_match_info_fetch will copy the matched substring for us.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_logical.c | 49 +++++++-------------------- 1 file changed, 13 insertions(+), 36 deletions(-)
I'm getting a build failure with this patch:
../../src/storage/storage_backend_logical.c:128:11: error: variable 'p' set but not used [-Werror=unused-but-set-variable] 128 | char *p = NULL;
Oops, another case where GCC reports a slightly different set of warnings than CLang.
Consider the following squashed in. Jano
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index d6c81e25b1..42dec05ba0 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -125,7 +125,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, g_autoptr(GMatchInfo) info = NULL; int nextents, ret = -1; const char *regex_unit = "(\\S+)\\((\\S+)\\)"; - char *p = NULL; size_t i; unsigned long long offset, size, length; virStorageVolSourceExtent extent; @@ -186,8 +185,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, goto cleanup; }
- p = groups[3]; - /* Each extent has a "path:offset" pair, and match #0 * is the whole matched string. */
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This saves us from allocating vars upfront, since GLib deals with that for us. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/vircommand.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 49432ddfcb..1ab3dbc819 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -22,7 +22,6 @@ #include <config.h> #include <poll.h> -#include <regex.h> #include <signal.h> #include <stdarg.h> #include <sys/stat.h> @@ -3077,11 +3076,10 @@ virCommandRunRegex(virCommandPtr cmd, const char *prefix, int *exitstatus) { - int err; - regex_t *reg; - g_autofree regmatch_t *vars = NULL; + GRegex **reg = NULL; + g_autoptr(GMatchInfo) info = NULL; size_t i, j, k; - int totgroups = 0, ngroup = 0, maxvars = 0; + int totgroups = 0, ngroup = 0; char **groups; g_autofree char *outbuf = NULL; VIR_AUTOSTRINGLIST lines = NULL; @@ -3092,29 +3090,23 @@ virCommandRunRegex(virCommandPtr cmd, return -1; for (i = 0; i < nregex; i++) { - err = regcomp(®[i], regex[i], REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, ®[i], error, sizeof(error)); + g_autoptr(GError) err = NULL; + reg[i] = g_regex_new(regex[i], G_REGEX_OPTIMIZE, 0, &err); + if (!reg[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), error); + _("Failed to compile regex %s"), err->message); for (j = 0; j < i; j++) - regfree(®[j]); + g_regex_unref(reg[j]); VIR_FREE(reg); return -1; } totgroups += nvars[i]; - if (nvars[i] > maxvars) - maxvars = nvars[i]; - } /* Storage for matched variables */ if (VIR_ALLOC_N(groups, totgroups) < 0) goto cleanup; - if (VIR_ALLOC_N(vars, maxvars+1) < 0) - goto cleanup; virCommandSetOutputBuffer(cmd, &outbuf); if (virCommandRun(cmd, exitstatus) < 0) @@ -3140,15 +3132,12 @@ virCommandRunRegex(virCommandPtr cmd, ngroup = 0; for (i = 0; i < nregex; i++) { - if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0) + if (!(g_regex_match(reg[i], p, 0, &info))) break; - /* NB vars[0] is the full pattern, so we offset j by 1 */ - for (j = 1; j <= nvars[i]; j++) { - if (VIR_STRNDUP(groups[ngroup++], p + vars[j].rm_so, - vars[j].rm_eo - vars[j].rm_so) < 0) - goto cleanup; - } + /* NB match #0 is the full pattern, so we offset j by 1 */ + for (j = 1; j <= nvars[i]; j++) + groups[ngroup++] = g_match_info_fetch(info, j); } /* We've matched on the last regex, so callback time */ @@ -3170,7 +3159,7 @@ virCommandRunRegex(virCommandPtr cmd, } for (i = 0; i < nregex; i++) - regfree(®[i]); + g_regex_unref(reg[i]); VIR_FREE(reg); return ret; -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:49 +0100, Ján Tomko wrote:
This saves us from allocating vars upfront, since GLib deals with that for us.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/vircommand.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 49432ddfcb..1ab3dbc819 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -22,7 +22,6 @@ #include <config.h>
#include <poll.h> -#include <regex.h> #include <signal.h> #include <stdarg.h> #include <sys/stat.h> @@ -3077,11 +3076,10 @@ virCommandRunRegex(virCommandPtr cmd, const char *prefix, int *exitstatus) { - int err; - regex_t *reg; - g_autofree regmatch_t *vars = NULL; + GRegex **reg = NULL; + g_autoptr(GMatchInfo) info = NULL;
This g_autoptr ...
size_t i, j, k; - int totgroups = 0, ngroup = 0, maxvars = 0; + int totgroups = 0, ngroup = 0; char **groups; g_autofree char *outbuf = NULL; VIR_AUTOSTRINGLIST lines = NULL; @@ -3092,29 +3090,23 @@ virCommandRunRegex(virCommandPtr cmd, return -1;
for (i = 0; i < nregex; i++) { - err = regcomp(®[i], regex[i], REG_EXTENDED); - if (err != 0) { - char error[100]; - regerror(err, ®[i], error, sizeof(error)); + g_autoptr(GError) err = NULL; + reg[i] = g_regex_new(regex[i], G_REGEX_OPTIMIZE, 0, &err); + if (!reg[i]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to compile regex %s"), error); + _("Failed to compile regex %s"), err->message); for (j = 0; j < i; j++) - regfree(®[j]); + g_regex_unref(reg[j]); VIR_FREE(reg); return -1; }
totgroups += nvars[i]; - if (nvars[i] > maxvars) - maxvars = nvars[i]; - }
/* Storage for matched variables */ if (VIR_ALLOC_N(groups, totgroups) < 0) goto cleanup; - if (VIR_ALLOC_N(vars, maxvars+1) < 0) - goto cleanup;
virCommandSetOutputBuffer(cmd, &outbuf); if (virCommandRun(cmd, exitstatus) < 0) @@ -3140,15 +3132,12 @@ virCommandRunRegex(virCommandPtr cmd,
ngroup = 0; for (i = 0; i < nregex; i++) { - if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0) + if (!(g_regex_match(reg[i], p, 0, &info)))
... will not free the pointer returned here overwritten by the iterating. The best will be to declare it just above.
break;
- /* NB vars[0] is the full pattern, so we offset j by 1 */ - for (j = 1; j <= nvars[i]; j++) { - if (VIR_STRNDUP(groups[ngroup++], p + vars[j].rm_so, - vars[j].rm_eo - vars[j].rm_so) < 0) - goto cleanup; - } + /* NB match #0 is the full pattern, so we offset j by 1 */ + for (j = 1; j <= nvars[i]; j++) + groups[ngroup++] = g_match_info_fetch(info, j);
} /* We've matched on the last regex, so callback time */ @@ -3170,7 +3159,7 @@ virCommandRunRegex(virCommandPtr cmd, }
for (i = 0; i < nregex; i++) - regfree(®[i]); + g_regex_unref(reg[i]);
VIR_FREE(reg); return ret;
with the memleak fixed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virlog.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index b3460d85fe..47e77e63b7 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -28,7 +28,6 @@ #include <sys/stat.h> #include <fcntl.h> #include <unistd.h> -#include <regex.h> #include <sys/uio.h> #if HAVE_SYSLOG_H # include <syslog.h> @@ -61,7 +60,7 @@ VIR_LOG_INIT("util.log"); -static regex_t *virLogRegex; +static GRegex *virLogRegex; static char virLogHostname[HOST_NAME_MAX+1]; @@ -268,10 +267,7 @@ virLogOnceInit(void) virLogLock(); virLogDefaultPriority = VIR_LOG_DEFAULT; - if (VIR_ALLOC_QUIET(virLogRegex) >= 0) { - if (regcomp(virLogRegex, VIR_LOG_REGEX, REG_EXTENDED) != 0) - VIR_FREE(virLogRegex); - } + virLogRegex = g_regex_new(VIR_LOG_REGEX, G_REGEX_OPTIMIZE, 0, NULL); /* We get and remember the hostname early, because at later time * it might not be possible to load NSS modules via getaddrinfo() @@ -1262,12 +1258,11 @@ virLogSetFromEnv(void) */ bool virLogProbablyLogMessage(const char *str) { - bool ret = false; if (!virLogRegex) return false; - if (regexec(virLogRegex, str, 0, NULL, 0) == 0) - ret = true; - return ret; + if (g_regex_match(virLogRegex, str, 0, NULL)) + return true; + return false; } -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:50 +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virlog.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virstring.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 283cf8c8d8..3da793c87a 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1017,29 +1017,27 @@ virStringSearch(const char *str, size_t max_matches, char ***matches) { - regex_t re; - regmatch_t rem; + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL; + g_autoptr(GMatchInfo) info = NULL; size_t nmatches = 0; ssize_t ret = -1; - int rv = -1; *matches = NULL; VIR_DEBUG("search '%s' for '%s'", str, regexp); - if ((rv = regcomp(&re, regexp, REG_EXTENDED)) != 0) { - char error[100]; - regerror(rv, &re, error, sizeof(error)); + regex = g_regex_new(regexp, 0, 0, &err); + if (!regex) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Error while compiling regular expression '%s': %s"), - regexp, error); + _("Failed to compile regex %s"), err->message); return -1; } - if (re.re_nsub != 1) { + if (g_regex_get_capture_count(regex) != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Regular expression '%s' must have exactly 1 match group, not %zu"), - regexp, re.re_nsub); + _("Regular expression '%s' must have exactly 1 match group, not %d"), + regexp, g_regex_get_capture_count(regex)); goto cleanup; } @@ -1051,28 +1049,27 @@ virStringSearch(const char *str, while ((nmatches - 1) < max_matches) { char *match; + int endpos; - if (regexec(&re, str, 1, &rem, 0) != 0) + if (!g_regex_match(regex, str, 0, &info)) break; if (VIR_EXPAND_N(*matches, nmatches, 1) < 0) goto cleanup; - if (VIR_STRNDUP(match, str + rem.rm_so, - rem.rm_eo - rem.rm_so) < 0) - goto cleanup; + match = g_match_info_fetch(info, 1); VIR_DEBUG("Got '%s'", match); (*matches)[nmatches-2] = match; - str = str + rem.rm_eo; + g_match_info_fetch_pos(info, 1, NULL, &endpos); + str += endpos; } ret = nmatches - 1; /* don't count the trailing null */ cleanup: - regfree(&re); if (ret < 0) { virStringListFree(*matches); *matches = NULL; -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:51 +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virstring.c | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c index 283cf8c8d8..3da793c87a 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1017,29 +1017,27 @@ virStringSearch(const char *str, size_t max_matches, char ***matches) { - regex_t re; - regmatch_t rem; + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL; + g_autoptr(GMatchInfo) info = NULL; size_t nmatches = 0; ssize_t ret = -1; - int rv = -1;
*matches = NULL;
VIR_DEBUG("search '%s' for '%s'", str, regexp);
- if ((rv = regcomp(&re, regexp, REG_EXTENDED)) != 0) { - char error[100]; - regerror(rv, &re, error, sizeof(error)); + regex = g_regex_new(regexp, 0, 0, &err); + if (!regex) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Error while compiling regular expression '%s': %s"), - regexp, error); + _("Failed to compile regex %s"), err->message); return -1; }
- if (re.re_nsub != 1) { + if (g_regex_get_capture_count(regex) != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Regular expression '%s' must have exactly 1 match group, not %zu"), - regexp, re.re_nsub); + _("Regular expression '%s' must have exactly 1 match group, not %d"), + regexp, g_regex_get_capture_count(regex)); goto cleanup; }
@@ -1051,28 +1049,27 @@ virStringSearch(const char *str,
while ((nmatches - 1) < max_matches) { char *match; + int endpos;
- if (regexec(&re, str, 1, &rem, 0) != 0) + if (!g_regex_match(regex, str, 0, &info))
info is overwritten and leaked
break;
if (VIR_EXPAND_N(*matches, nmatches, 1) < 0) goto cleanup;
- if (VIR_STRNDUP(match, str + rem.rm_so, - rem.rm_eo - rem.rm_so) < 0) - goto cleanup; + match = g_match_info_fetch(info, 1);
VIR_DEBUG("Got '%s'", match);
(*matches)[nmatches-2] = match;
- str = str + rem.rm_eo; + g_match_info_fetch_pos(info, 1, NULL, &endpos); + str += endpos; }
ret = nmatches - 1; /* don't count the trailing null */
cleanup: - regfree(&re); if (ret < 0) { virStringListFree(*matches); *matches = NULL;
With the memleak resolved: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virstring.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 3da793c87a..6add2278d8 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -19,7 +19,6 @@ #include <config.h> #include <glib/gprintf.h> -#include <regex.h> #include <locale.h> #include "c-ctype.h" @@ -1089,24 +1088,19 @@ bool virStringMatch(const char *str, const char *regexp) { - regex_t re; - int rv; + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL; VIR_DEBUG("match '%s' for '%s'", str, regexp); - if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) { - char error[100]; - regerror(rv, &re, error, sizeof(error)); - VIR_WARN("error while compiling regular expression '%s': %s", - regexp, error); - return false; + regex = g_regex_new(regexp, 0, 0, &err); + if (!regex) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to compile regex %s"), err->message); + return -1; } - rv = regexec(&re, str, 0, NULL, 0); - - regfree(&re); - - return rv == 0; + return g_regex_match(regex, str, 0, NULL); } /** -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:52 +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virstring.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/util/virstring.c b/src/util/virstring.c index 3da793c87a..6add2278d8 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -19,7 +19,6 @@ #include <config.h>
#include <glib/gprintf.h> -#include <regex.h> #include <locale.h>
#include "c-ctype.h" @@ -1089,24 +1088,19 @@ bool virStringMatch(const char *str, const char *regexp) { - regex_t re; - int rv; + g_autoptr(GRegex) regex = NULL; + g_autoptr(GError) err = NULL;
VIR_DEBUG("match '%s' for '%s'", str, regexp);
- if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) { - char error[100]; - regerror(rv, &re, error, sizeof(error)); - VIR_WARN("error while compiling regular expression '%s': %s", - regexp, error); - return false; + regex = g_regex_new(regexp, 0, 0, &err); + if (!regex) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to compile regex %s"), err->message); + return -1;
The function returns bool. Additionally there is one non-test use of this function. Do you think we should open-code it instead?
}
- rv = regexec(&re, str, 0, NULL, 0); - - regfree(&re); - - return rv == 0; + return g_regex_match(regex, str, 0, NULL); }
With the return value/error case sorted out: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/vboxsnapshotxmltest.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/vboxsnapshotxmltest.c b/tests/vboxsnapshotxmltest.c index 8faf2b9c26..d1a7522931 100644 --- a/tests/vboxsnapshotxmltest.c +++ b/tests/vboxsnapshotxmltest.c @@ -4,7 +4,6 @@ #ifdef WITH_VBOX -# include <regex.h> # include "vbox/vbox_snapshot_conf.h" # define VIR_FROM_THIS VIR_FROM_NONE @@ -12,7 +11,7 @@ static const char *testSnapshotXMLVariableLineRegexStr = "lastStateChange=[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z"; -regex_t *testSnapshotXMLVariableLineRegex = NULL; +GRegex *testSnapshotXMLVariableLineRegex = NULL; static char * testFilterXML(char *xml) @@ -29,8 +28,7 @@ testFilterXML(char *xml) VIR_FREE(xml); for (xmlLine = xmlLines; *xmlLine; xmlLine++) { - if (regexec(testSnapshotXMLVariableLineRegex, - *xmlLine, 0, NULL, 0) == 0) + if (g_regex_match(testSnapshotXMLVariableLineRegex, *xmlLine, 0, NULL)) continue; virBufferStrcat(&buf, *xmlLine, "\n", NULL); @@ -112,12 +110,12 @@ static int mymain(void) { int ret = 0; - if (VIR_ALLOC(testSnapshotXMLVariableLineRegex) < 0) - goto cleanup; + g_autoptr(GError) err = NULL; + + testSnapshotXMLVariableLineRegex = g_regex_new(testSnapshotXMLVariableLineRegexStr, + 0, 0, &err); - if (regcomp(testSnapshotXMLVariableLineRegex, - testSnapshotXMLVariableLineRegexStr, - REG_EXTENDED | REG_NOSUB) != 0) { + if (!testSnapshotXMLVariableLineRegex) { ret = -1; virReportError(VIR_ERR_INTERNAL_ERROR, "%s", "failed to compile test regex"); @@ -136,9 +134,7 @@ mymain(void) DO_TEST("2disks-3snap-brother"); cleanup: - if (testSnapshotXMLVariableLineRegex) - regfree(testSnapshotXMLVariableLineRegex); - VIR_FREE(testSnapshotXMLVariableLineRegex); + g_regex_unref(testSnapshotXMLVariableLineRegex); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.21.0
On Wed, Nov 13, 2019 at 16:48:53 +0100, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tests/vboxsnapshotxmltest.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Ján Tomko -
Peter Krempa