1:37 a.m.
Change libxlGetAutoballoonConf() function to return an int
for success/failure, and fail if regcomp fails.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_conf.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index fcb278b..a634476 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1014,21 +1014,28 @@ error:
return -1;
}
-static bool
-libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
+static int
+libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, bool *autoballoon)
{
regex_t regex;
- int ret;
+ int res;
+
+ 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));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to compile regex %s"),
+ error);
- ret = regcomp(®ex,
- "(^| )dom0_mem=((|min:|max:)[0-9]+[bBkKmMgG]?,?)+($| )",
- REG_NOSUB | REG_EXTENDED);
- if (ret)
- return true;
+ return -1;
+ }
- ret = regexec(®ex, cfg->verInfo->commandline, 0, NULL, 0);
+ res = regexec(®ex, cfg->verInfo->commandline, 0, NULL, 0);
regfree(®ex);
- return ret == REG_NOMATCH;
+ *autoballoon = res == REG_NOMATCH;
+ return 0;
}
libxlDriverConfigPtr
@@ -1098,7 +1105,8 @@ libxlDriverConfigNew(void)
}
/* setup autoballoon */
- cfg->autoballoon = libxlGetAutoballoonConf(cfg);
+ if (libxlGetAutoballoonConf(cfg, &cfg->autoballoon) < 0)
+ goto error;
return cfg;
--
1.8.1.4
3:29 a.m.
On 04.09.2013 08:37, Jim Fehlig wrote:
Change libxlGetAutoballoonConf() function to return an int
for success/failure, and fail if regcomp fails.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/libxl/libxl_conf.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index fcb278b..a634476 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1014,21 +1014,28 @@ error:
return -1;
}
-static bool
-libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
+static int
+libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, bool *autoballoon)
{
regex_t regex;
- int ret;
+ int res;
+
+ 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));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to compile regex %s"),
+ error);
My only concern is, that 100 bytes may not be enough sometimes. But on
the other hand, there's another occurrence of regerror and there's
buffer of the exact size passed too. So I'm comfortable enough to give
you my ACK.
Michal
3:37 a.m.
On 04.09.2013 10:29, Michal Privoznik wrote:
On 04.09.2013 08:37, Jim Fehlig wrote:
> Change libxlGetAutoballoonConf() function to return an int
> for success/failure, and fail if regcomp fails.
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> src/libxl/libxl_conf.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index fcb278b..a634476 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1014,21 +1014,28 @@ error:
> return -1;
> }
>
> -static bool
> -libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
> +static int
> +libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, bool *autoballoon)
> {
> regex_t regex;
> - int ret;
> + int res;
> +
> + 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));
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to compile regex %s"),
> + error);
My only concern is, that 100 bytes may not be enough sometimes. But on
the other hand, there's another occurrence of regerror and there's
buffer of the exact size passed too. So I'm comfortable enough to give
you my ACK.
Hit send prematurely, the other occurrence is doing regfree(®ex);
even on error path (in fact, just on the error path). I must confess I
don't get it (in case of error, there should not be any regex created,
should it?). So - do we need to regfree() even on error?
Michal
11:45 a.m.
Michal Privoznik wrote:
On 04.09.2013 10:29, Michal Privoznik wrote:
> On 04.09.2013 08:37, Jim Fehlig wrote:
>
>> Change libxlGetAutoballoonConf() function to return an int
>> for success/failure, and fail if regcomp fails.
>>
>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>> ---
>> src/libxl/libxl_conf.c | 30 +++++++++++++++++++-----------
>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index fcb278b..a634476 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -1014,21 +1014,28 @@ error:
>> return -1;
>> }
>>
>> -static bool
>> -libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
>> +static int
>> +libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, bool *autoballoon)
>> {
>> regex_t regex;
>> - int ret;
>> + int res;
>> +
>> + 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));
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Failed to compile regex %s"),
>> + error);
>>
> My only concern is, that 100 bytes may not be enough sometimes. But on
> the other hand, there's another occurrence of regerror and there's
> buffer of the exact size passed too. So I'm comfortable enough to give
> you my ACK.
>
>
Hit send prematurely, the other occurrence is doing regfree(®ex);
even on error path (in fact, just on the error path). I must confess I
don't get it (in case of error, there should not be any regex created,
should it?). So - do we need to regfree() even on error?
Yeah, good question. I found a few occurrences of regcomp() and friends
throughout the sources and most seem to do regfree() even when regcomp()
fails. The man page is not very clear, but the notes on regfree()
suggest it is not necessary
POSIX Pattern Buffer Freeing
Supplying regfree() with a precompiled pattern buffer, preg will
free the memory allocated to the pattern buffer by the compiling
process, regcomp().
But does the pattern buffer contain any allocated memory when regcomp()
fails? The notes on regcomp() are not clear about this.
Regards,
Jim
3:03 p.m.
Jim Fehlig wrote:
Michal Privoznik wrote:
> On 04.09.2013 10:29, Michal Privoznik wrote:
>
>
>> On 04.09.2013 08:37, Jim Fehlig wrote:
>>
>>
>>> Change libxlGetAutoballoonConf() function to return an int
>>> for success/failure, and fail if regcomp fails.
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>>> ---
>>> src/libxl/libxl_conf.c | 30 +++++++++++++++++++-----------
>>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index fcb278b..a634476 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -1014,21 +1014,28 @@ error:
>>> return -1;
>>> }
>>>
>>> -static bool
>>> -libxlGetAutoballoonConf(libxlDriverConfigPtr cfg)
>>> +static int
>>> +libxlGetAutoballoonConf(libxlDriverConfigPtr cfg, bool *autoballoon)
>>> {
>>> regex_t regex;
>>> - int ret;
>>> + int res;
>>> +
>>> + 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));
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("Failed to compile regex %s"),
>>> + error);
>>>
>>>
>> My only concern is, that 100 bytes may not be enough sometimes. But on
>> the other hand, there's another occurrence of regerror and there's
>> buffer of the exact size passed too. So I'm comfortable enough to give
>> you my ACK.
>>
>>
>>
> Hit send prematurely, the other occurrence is doing regfree(®ex);
> even on error path (in fact, just on the error path). I must confess I
> don't get it (in case of error, there should not be any regex created,
> should it?). So - do we need to regfree() even on error?
>
>
Yeah, good question. I found a few occurrences of regcomp() and friends
throughout the sources and most seem to do regfree() even when regcomp()
fails. The man page is not very clear, but the notes on regfree()
suggest it is not necessary
POSIX Pattern Buffer Freeing
Supplying regfree() with a precompiled pattern buffer, preg will
free the memory allocated to the pattern buffer by the compiling
process, regcomp().
But does the pattern buffer contain any allocated memory when regcomp()
fails? The notes on regcomp() are not clear about this.
The System Interfaces volume of POSIX.1-2008 [1] says this about
regcomp() return value
Upon successful completion, the regcomp() function shall return 0.
Otherwise, it shall return an integer value indicating an error as
described in /<regex.h>/
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>;,
and the content of preg is undefined. If a code is returned, the
interpretation shall be as given in /<regex.h>/
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>;.
I don't think we want to call regfree() on an undefined preg right?
Regards,
Jim
[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html
3:18 p.m.
On 09/04/2013 02:03 PM, Jim Fehlig wrote:
>
> Yeah, good question. I found a few occurrences of regcomp() and friends
> throughout the sources and most seem to do regfree() even when regcomp()
> fails. The man page is not very clear, but the notes on regfree()
> suggest it is not necessary
>
> POSIX Pattern Buffer Freeing
> Supplying regfree() with a precompiled pattern buffer, preg will
> free the memory allocated to the pattern buffer by the compiling
> process, regcomp().
>
> But does the pattern buffer contain any allocated memory when regcomp()
> fails? The notes on regcomp() are not clear about this.
Thankfully, we can read the source :)
In glibc, regcomp assigns into preg, but is careful to undo any
allocation on failure; it is also careful to make regfree() a no-op on
an already-freed buffer (whether by calling regfree() twice in a row, or
using it on preg after a failed regcomp). Gnulib copies this behavior.
But it is not universally standard:
>
The System Interfaces volume of POSIX.1-2008 [1] says this about
regcomp() return value
Upon successful completion, the regcomp() function shall return 0.
Otherwise, it shall return an integer value indicating an error as
described in /<regex.h>/
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>;,
and the content of preg is undefined. If a code is returned, the
interpretation shall be as given in /<regex.h>/
<http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>;.
I don't think we want to call regfree() on an undefined preg right?
Correct - regfree() is only needed on successful regcomp(). We can
probably get away with calling regfree() even on failure because we use
gnulib, but that's not a good reason, so it wouldn't hurt to audit the
code and guarantee a free only on success.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:57 p.m.
Eric Blake wrote:
On 09/04/2013 02:03 PM, Jim Fehlig wrote:
>> Yeah, good question. I found a few occurrences of regcomp() and friends
>> throughout the sources and most seem to do regfree() even when regcomp()
>> fails. The man page is not very clear, but the notes on regfree()
>> suggest it is not necessary
>>
>> POSIX Pattern Buffer Freeing
>> Supplying regfree() with a precompiled pattern buffer, preg will
>> free the memory allocated to the pattern buffer by the compiling
>> process, regcomp().
>>
>> But does the pattern buffer contain any allocated memory when regcomp()
>> fails? The notes on regcomp() are not clear about this.
>>
Thankfully, we can read the source :)
Nod :). Was about to do that before seeing your message...
In glibc, regcomp assigns into preg, but is careful to undo any
allocation on failure; it is also careful to make regfree() a no-op on
an already-freed buffer (whether by calling regfree() twice in a row, or
using it on preg after a failed regcomp). Gnulib copies this behavior.
But it is not universally standard:
>>
>>
> The System Interfaces volume of POSIX.1-2008 [1] says this about
> regcomp() return value
>
> Upon successful completion, the regcomp() function shall return 0.
> Otherwise, it shall return an integer value indicating an error as
> described in /<regex.h>/
> <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>;,
> and the content of preg is undefined. If a code is returned, the
> interpretation shall be as given in /<regex.h>/
> <http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/regex.h.html>;.
>
> I don't think we want to call regfree() on an undefined preg right?
>
Correct - regfree() is only needed on successful regcomp(). We can
probably get away with calling regfree() even on failure because we use
gnulib, but that's not a good reason, so it wouldn't hurt to audit the
code and guarantee a free only on success.
I've pushed this patch, given Michal's ACK before raising the regfree()
question.
Also sent a small series to remove other unnecessary uses of regfree()
https://www.redhat.com/archives/libvir-list/2013-September/msg00272.html
Regards,
Jim
2:49 a.m.
On 04.09.2013 22:18, Eric Blake wrote:
On 09/04/2013 02:03 PM, Jim Fehlig wrote:
>>
>> Yeah, good question. I found a few occurrences of regcomp() and friends
>> throughout the sources and most seem to do regfree() even when regcomp()
>> fails. The man page is not very clear, but the notes on regfree()
>> suggest it is not necessary
>>
>> POSIX Pattern Buffer Freeing
>> Supplying regfree() with a precompiled pattern buffer, preg will
>> free the memory allocated to the pattern buffer by the compiling
>> process, regcomp().
>>
>> But does the pattern buffer contain any allocated memory when regcomp()
>> fails? The notes on regcomp() are not clear about this.
Thankfully, we can read the source :)
In glibc, regcomp assigns into preg, but is careful to undo any
allocation on failure; it is also careful to make regfree() a no-op on
an already-freed buffer (whether by calling regfree() twice in a row, or
using it on preg after a failed regcomp). Gnulib copies this behavior.
But it is not universally standard:
Question then is: if regcomp(®, ...) fails, reg == NULL, right? But
passing the @reg to regerror: regerror(errcode, ®, ...) doesn't make
much sense then. It can make, if regcomp would free all mem allocated,
but set @reg to different values (each of them representing different
cause of error). If that is the case, <irony>this is what I call the
design!</irony>
Michal
8:01 a.m.
On 09/05/2013 01:49 AM, Michal Privoznik wrote:
> In glibc, regcomp assigns into preg, but is careful to undo any
> allocation on failure; it is also careful to make regfree() a no-op on
> an already-freed buffer (whether by calling regfree() twice in a row, or
> using it on preg after a failed regcomp). Gnulib copies this behavior.
> But it is not universally standard:
Question then is: if regcomp(®, ...) fails, reg == NULL, right?
No. You, the caller, supplied reg (typically stack allocated). As in:
regex_t reg;
int ret;
if ((ret = regcomp(®, str, flags))) {
regerror(ret, ®, buf, len);
} else {
regexec(...)
...
regfree(®);
}
The only thing that regfree() does is free any pointers _within_ the
regex_t struct. Remember, regex_t is a semi-opaque struct - POSIX only
documents that it contains a re_nsub member (and portable code therefore
cannot use any of its other members directly), but you ARE promised that
it is not an incomplete type, and that it is larger than a simple
pointer. It is _because_ regex_t is a struct with non-pointer members,
and where _you_ allocate the storage for the struct, that the struct can
contain useful information even after a failed regcomp() so that
regerror(®) can give a nicer message than regerror(NULL). But
ultimately, as long as regcomp() leaves no allocated data in the
remaining unnamed members, then there is nothing for regfree() to clean
up after failure.
You know, maybe I should ask the Austin Group (the people in charge of
POSIX) to clarify whether regfree() can/must be used after regcomp
failure, since the current wording isn't very clear.
But
passing the @reg to regerror: regerror(errcode, ®, ...) doesn't make
much sense then. It can make, if regcomp would free all mem allocated,
but set @reg to different values (each of them representing different
cause of error). If that is the case, <irony>this is what I call the
design!</irony>
Michal
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4097
days inactive
4098
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
Jim Fehlig -
Michal Privoznik