5:33 p.m.
During a recent discussion about calling regfree() when regcomp() fails,
we decided it was best to not call regfree() when compilation fails.
https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html
The first patch fixes existing infractions throughout the code. I split
the second patch out since there were other issues beyond simply removing
a call to regfree().
Jim Fehlig (2):
Don't call regfree() if regcomp() fails
libxl: Compile regular expression where it is used
src/libxl/libxl_conf.c | 29 +++++++++++++++--------------
src/storage/storage_backend.c | 2 +-
src/storage/storage_backend_logical.c | 1 -
src/xen/xen_hypervisor.c | 3 ---
4 files changed, 16 insertions(+), 19 deletions(-)
--
1.8.1.4
5:33 p.m.
New subject: [libvirt] [PATCH 1/2] Don't call regfree() if regcomp() fails
POSIX states that the preg parameter to regcomp() is undefined on
failure, so no need to call regfree() in these cases.
http://pubs.opengroup.org/onlinepubs/009695399/functions/regcomp.html
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/storage/storage_backend.c | 2 +-
src/storage/storage_backend_logical.c | 1 -
src/xen/xen_hypervisor.c | 3 ---
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 4ebe11b..bbdda19 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1579,7 +1579,7 @@ virStorageBackendRunProgRegex(virStoragePoolObjPtr pool,
regerror(err, ®[i], error, sizeof(error));
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to compile regex %s"), error);
- for (j = 0; j <= i; j++)
+ for (j = 0; j < i; j++)
regfree(®[j]);
VIR_FREE(reg);
return -1;
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 8998a11..0b14679 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -195,7 +195,6 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool,
if (err != 0) {
char error[100];
regerror(err, reg, error, sizeof(error));
- regfree(reg);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to compile regex %s"),
error);
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index cd85b75..f9c7b40 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1787,7 +1787,6 @@ xenHypervisorInit(struct xenHypervisorVersions *override_versions)
if (errcode != 0) {
char error[100];
regerror(errcode, &flags_hvm_rec, error, sizeof(error));
- regfree(&flags_hvm_rec);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", error);
return -1;
}
@@ -1795,7 +1794,6 @@ xenHypervisorInit(struct xenHypervisorVersions *override_versions)
if (errcode != 0) {
char error[100];
regerror(errcode, &flags_pae_rec, error, sizeof(error));
- regfree(&flags_pae_rec);
regfree(&flags_hvm_rec);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", error);
return -1;
@@ -1804,7 +1802,6 @@ xenHypervisorInit(struct xenHypervisorVersions *override_versions)
if (errcode != 0) {
char error[100];
regerror(errcode, &xen_cap_rec, error, sizeof(error));
- regfree(&xen_cap_rec);
regfree(&flags_pae_rec);
regfree(&flags_hvm_rec);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", error);
--
1.8.1.4
5:33 p.m.
New subject: [libvirt] [PATCH 2/2] libxl: Compile regular expression where it is used
The regular expression used to determine guest capabilities
was compiled in libxlCapsInitHost() but used in libxlCapsInitGuests().
Move compilation to libxlCapsInitGuests() where it is used, and free
the compiled regex after use. Ensure not to free the regex if
compilation fails.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_conf.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index fcb278b..c7be7aa 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -61,8 +61,7 @@ struct guest_arch {
int ia64_be;
};
-static const char *xen_cap_re =
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
-static regex_t xen_cap_rec;
+#define XEN_CAP_REGEX
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
static virClassPtr libxlDriverConfigClass;
@@ -103,20 +102,9 @@ libxlDriverConfigDispose(void *obj)
static int
libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps)
{
- int err;
libxl_physinfo phy_info;
int host_pae;
- err = regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED);
- if (err != 0) {
- char error[100];
- regerror(err, &xen_cap_rec, error, sizeof(error));
- regfree(&xen_cap_rec);
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to compile regex %s"), error);
- return -1;
- }
-
if (libxl_get_physinfo(ctx, &phy_info) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to get node physical info from
libxenlight"));
@@ -249,6 +237,8 @@ static int
libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
{
const libxl_version_info *ver_info;
+ int err;
+ regex_t regex;
char *str, *token;
regmatch_t subs[4];
char *saveptr = NULL;
@@ -265,6 +255,16 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
_("Failed to get version info from libxenlight"));
return -1;
}
+
+ err = regcomp(®ex, XEN_CAP_REGEX, REG_EXTENDED);
+ if (err != 0) {
+ char error[100];
+ regerror(err, ®ex, error, sizeof(error));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to compile regex %s"), error);
+ return -1;
+ }
+
/* Format of capabilities string is documented in the code in
* xen-unstable.hg/xen/arch/.../setup.c.
*
@@ -294,7 +294,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
&& (token = strtok_r(str, " ", &saveptr)) !=
NULL;
str = NULL) {
- if (regexec(&xen_cap_rec, token, sizeof(subs) / sizeof(subs[0]),
+ if (regexec(®ex, token, sizeof(subs) / sizeof(subs[0]),
subs, 0) == 0) {
int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm");
virArch arch;
@@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
guest_archs[i].ia64_be = ia64_be;
}
}
+ regfree(®ex);
for (i = 0; i < nr_guest_archs; ++i) {
virCapsGuestPtr guest;
--
1.8.1.4
5:40 p.m.
New subject: [libvirt] [PATCH 2/2] libxl: Compile regular expression where it is used
Jim Fehlig wrote:
The regular expression used to determine guest capabilities
was compiled in libxlCapsInitHost() but used in libxlCapsInitGuests().
Move compilation to libxlCapsInitGuests() where it is used, and free
the compiled regex after use. Ensure not to free the regex if
compilation fails.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_conf.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index fcb278b..c7be7aa 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -61,8 +61,7 @@ struct guest_arch {
int ia64_be;
};
-static const char *xen_cap_re =
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
-static regex_t xen_cap_rec;
+#define XEN_CAP_REGEX
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
Opps, that's not quite right. The version I actually compiled and
tested doesn't have the trailing ';' on the #define :).
Regards,
Jim
static virClassPtr libxlDriverConfigClass;
@@ -103,20 +102,9 @@ libxlDriverConfigDispose(void *obj)
static int
libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps)
{
- int err;
libxl_physinfo phy_info;
int host_pae;
- err = regcomp(&xen_cap_rec, xen_cap_re, REG_EXTENDED);
- if (err != 0) {
- char error[100];
- regerror(err, &xen_cap_rec, error, sizeof(error));
- regfree(&xen_cap_rec);
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Failed to compile regex %s"), error);
- return -1;
- }
-
if (libxl_get_physinfo(ctx, &phy_info) != 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to get node physical info from
libxenlight"));
@@ -249,6 +237,8 @@ static int
libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
{
const libxl_version_info *ver_info;
+ int err;
+ regex_t regex;
char *str, *token;
regmatch_t subs[4];
char *saveptr = NULL;
@@ -265,6 +255,16 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
_("Failed to get version info from libxenlight"));
return -1;
}
+
+ err = regcomp(®ex, XEN_CAP_REGEX, REG_EXTENDED);
+ if (err != 0) {
+ char error[100];
+ regerror(err, ®ex, error, sizeof(error));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to compile regex %s"), error);
+ return -1;
+ }
+
/* Format of capabilities string is documented in the code in
* xen-unstable.hg/xen/arch/.../setup.c.
*
@@ -294,7 +294,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
&& (token = strtok_r(str, " ", &saveptr)) !=
NULL;
str = NULL) {
- if (regexec(&xen_cap_rec, token, sizeof(subs) / sizeof(subs[0]),
+ if (regexec(®ex, token, sizeof(subs) / sizeof(subs[0]),
subs, 0) == 0) {
int hvm = STRPREFIX(&token[subs[1].rm_so], "hvm");
virArch arch;
@@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
guest_archs[i].ia64_be = ia64_be;
}
}
+ regfree(®ex);
for (i = 0; i < nr_guest_archs; ++i) {
virCapsGuestPtr guest;
4:20 a.m.
New subject: [libvirt] [PATCH 2/2] libxl: Compile regular expression where it is used
On 05.09.2013 00:40, Jim Fehlig wrote:
Jim Fehlig wrote:
> The regular expression used to determine guest capabilities
> was compiled in libxlCapsInitHost() but used in libxlCapsInitGuests().
> Move compilation to libxlCapsInitGuests() where it is used, and free
> the compiled regex after use. Ensure not to free the regex if
> compilation fails.
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> src/libxl/libxl_conf.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index fcb278b..c7be7aa 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -61,8 +61,7 @@ struct guest_arch {
> int ia64_be;
> };
>
> -static const char *xen_cap_re =
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
> -static regex_t xen_cap_rec;
> +#define XEN_CAP_REGEX
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
>
Opps, that's not quite right. The version I actually compiled and
tested doesn't have the trailing ';' on the #define :).
Yep, gcc caught that. ACK series (with this change).
Michal
12:02 p.m.
New subject: [libvirt] [PATCH 2/2] libxl: Compile regular expression where it is used
Michal Privoznik wrote:
On 05.09.2013 00:40, Jim Fehlig wrote:
> Jim Fehlig wrote:
>
>> The regular expression used to determine guest capabilities
>> was compiled in libxlCapsInitHost() but used in libxlCapsInitGuests().
>> Move compilation to libxlCapsInitGuests() where it is used, and free
>> the compiled regex after use. Ensure not to free the regex if
>> compilation fails.
>>
>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>> ---
>> src/libxl/libxl_conf.c | 29 +++++++++++++++--------------
>> 1 file changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index fcb278b..c7be7aa 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -61,8 +61,7 @@ struct guest_arch {
>> int ia64_be;
>> };
>>
>> -static const char *xen_cap_re =
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
>> -static regex_t xen_cap_rec;
>> +#define XEN_CAP_REGEX
"(xen|hvm)-[[:digit:]]+\\.[[:digit:]]+-(x86_32|x86_64|ia64|powerpc64)(p|be)?";
>>
>>
> Opps, that's not quite right. The version I actually compiled and
> tested doesn't have the trailing ';' on the #define :).
>
>
Yep, gcc caught that. ACK series (with this change).
Thanks. I added link to the thread which inspired this series in patch
1 commit message and pushed.
Regards,
Jim
4097
days inactive
4098
days old
5 comments
2 participants
participants (2)
-
Jim Fehlig -
Michal Privoznik