[libvirt] [PATCH 0/5] virCommandRunRegex cleanups
Simplify the code. This should have no functional change (except for the impossible leak fix in 3/5). Most of this functionality is tested in viriscsitest. Ján Tomko (5): Remove useless 'maxReg' variable Simplify the loop in virCommandRunRegex Free groups in case of a partial match Use VIR_STRNDUP instead of modifying the matched string Shift the for loop over matched vars by one src/util/vircommand.c | 46 +++++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 27 deletions(-) -- 1.8.3.2
It is used to break out of the loop early if one regex does not match. Use the 'break' statement instead. --- src/util/vircommand.c | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 8250634..f1adb1e 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2782,7 +2782,6 @@ virCommandRunRegex(virCommandPtr cmd, int err; regex_t *reg; regmatch_t *vars = NULL; - int maxReg = 0; size_t i, j, k; int totgroups = 0, ngroup = 0, maxvars = 0; char **groups; @@ -2841,32 +2840,30 @@ virCommandRunRegex(virCommandPtr cmd, if (!p) p = lines[k]; - for (i = 0; i <= maxReg && i < nregex; i++) { - if (regexec(®[i], p, nvars[i]+1, vars, 0) == 0) { - maxReg++; + for (i = 0; i < nregex; i++) { + if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0) + break; - if (i == 0) - ngroup = 0; + if (i == 0) + ngroup = 0; - /* NULL terminate each captured group in the line */ - for (j = 0; j < nvars[i]; j++) { - /* NB vars[0] is the full pattern, so we offset j by 1 */ - p[vars[j+1].rm_eo] = '\0'; - if (VIR_STRDUP(groups[ngroup++], p + vars[j+1].rm_so) < 0) - goto cleanup; - } + /* NULL terminate each captured group in the line */ + for (j = 0; j < nvars[i]; j++) { + /* NB vars[0] is the full pattern, so we offset j by 1 */ + p[vars[j+1].rm_eo] = '\0'; + if (VIR_STRDUP(groups[ngroup++], p + vars[j+1].rm_so) < 0) + goto cleanup; + } - /* We're matching on the last regex, so callback time */ - if (i == (nregex-1)) { - if (((*func)(groups, data)) < 0) - goto cleanup; + /* We're matching on the last regex, so callback time */ + if (i == (nregex-1)) { + if (((*func)(groups, data)) < 0) + goto cleanup; - /* Release matches & restart to matching the first regex */ - for (j = 0; j < totgroups; j++) - VIR_FREE(groups[j]); - maxReg = 0; - ngroup = 0; - } + /* Release matches & restart to matching the first regex */ + for (j = 0; j < totgroups; j++) + VIR_FREE(groups[j]); + ngroup = 0; } } } -- 1.8.3.2
Do not check for border iterator values inside the loop, move the code before/after the loop instead. --- src/util/vircommand.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index f1adb1e..c17792b 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2840,13 +2840,11 @@ virCommandRunRegex(virCommandPtr cmd, if (!p) p = lines[k]; + ngroup = 0; for (i = 0; i < nregex; i++) { if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0) break; - if (i == 0) - ngroup = 0; - /* NULL terminate each captured group in the line */ for (j = 0; j < nvars[i]; j++) { /* NB vars[0] is the full pattern, so we offset j by 1 */ @@ -2855,16 +2853,14 @@ virCommandRunRegex(virCommandPtr cmd, goto cleanup; } - /* We're matching on the last regex, so callback time */ - if (i == (nregex-1)) { - if (((*func)(groups, data)) < 0) - goto cleanup; + } + /* We've matched on the last regex, so callback time */ + if (i == nregex) { + if (((*func)(groups, data)) < 0) + goto cleanup; - /* Release matches & restart to matching the first regex */ - for (j = 0; j < totgroups; j++) - VIR_FREE(groups[j]); - ngroup = 0; - } + for (j = 0; j < totgroups; j++) + VIR_FREE(groups[j]); } } -- 1.8.3.2
If there are more than two regexes, but only one of them matches, the matched groups would be leaked. --- src/util/vircommand.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c17792b..fab3aba 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2858,10 +2858,10 @@ virCommandRunRegex(virCommandPtr cmd, if (i == nregex) { if (((*func)(groups, data)) < 0) goto cleanup; - - for (j = 0; j < totgroups; j++) - VIR_FREE(groups[j]); } + + for (j = 0; j < ngroup; j++) + VIR_FREE(groups[j]); } ret = 0; -- 1.8.3.2
--- src/util/vircommand.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index fab3aba..7d13828 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2832,7 +2832,7 @@ virCommandRunRegex(virCommandPtr cmd, goto cleanup; for (k = 0; lines[k]; k++) { - char *p = NULL; + const char *p = NULL; /* ignore any command prefix */ if (prefix) @@ -2845,11 +2845,10 @@ virCommandRunRegex(virCommandPtr cmd, if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0) break; - /* NULL terminate each captured group in the line */ for (j = 0; j < nvars[i]; j++) { /* NB vars[0] is the full pattern, so we offset j by 1 */ - p[vars[j+1].rm_eo] = '\0'; - if (VIR_STRDUP(groups[ngroup++], p + vars[j+1].rm_so) < 0) + if (VIR_STRNDUP(groups[ngroup++], p + vars[j+1].rm_so, + vars[j+1].rm_eo - vars[j+1].rm_so) < 0) goto cleanup; } -- 1.8.3.2
Instead of adding one to the iterator on every use. --- src/util/vircommand.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 7d13828..a21a88b 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2845,10 +2845,10 @@ virCommandRunRegex(virCommandPtr cmd, if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0) break; - for (j = 0; j < nvars[i]; j++) { - /* NB vars[0] is the full pattern, so we offset j by 1 */ - if (VIR_STRNDUP(groups[ngroup++], p + vars[j+1].rm_so, - vars[j+1].rm_eo - vars[j+1].rm_so) < 0) + /* 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; } -- 1.8.3.2
On 25.03.2014 08:17, Ján Tomko wrote:
Simplify the code. This should have no functional change (except for the impossible leak fix in 3/5). Most of this functionality is tested in viriscsitest.
Ján Tomko (5): Remove useless 'maxReg' variable Simplify the loop in virCommandRunRegex Free groups in case of a partial match Use VIR_STRNDUP instead of modifying the matched string Shift the for loop over matched vars by one
src/util/vircommand.c | 46 +++++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 27 deletions(-)
ACK series. Michal
On 03/26/2014 02:20 PM, Michal Privoznik wrote:
On 25.03.2014 08:17, Ján Tomko wrote:
Simplify the code. This should have no functional change (except for the impossible leak fix in 3/5). Most of this functionality is tested in viriscsitest.
Ján Tomko (5): Remove useless 'maxReg' variable Simplify the loop in virCommandRunRegex Free groups in case of a partial match Use VIR_STRNDUP instead of modifying the matched string Shift the for loop over matched vars by one
src/util/vircommand.c | 46 +++++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 27 deletions(-)
ACK series.
Thanks; pushed now. Jan
participants (2)
-
Ján Tomko -
Michal Privoznik