Switch to using the 'g_auto*' helpers.
Signed-off-by: Yi Li <yili@winhong.com>
---
src/util/virconf.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 16107bce96..11046a1d45 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -1505,26 +1505,19 @@ virConfLoadConfigPath(const char *name)
int
virConfLoadConfig(virConfPtr *conf, const char *name)
{
- char *path = NULL;
- int ret = -1;
+ g_autofree char *path = NULL;
*conf = NULL;
if (!(path = virConfLoadConfigPath(name)))
- goto cleanup;
+ return -1;
- if (!virFileExists(path)) {
- ret = 0;
- goto cleanup;
- }
+ if (!virFileExists(path))
+ return 0;
VIR_DEBUG("Loading config file '%s'", path);
if (!(*conf = virConfReadFile(path, 0)))
- goto cleanup;
-
- ret = 0;
+ return -1;
- cleanup:
- VIR_FREE(path);
- return ret;
+ return 0;
}
--
2.25.3
On 3/11/21 10:46 AM, Yi Li wrote: > Switch to using the 'g_auto*' helpers. > > Signed-off-by: Yi Li <yili@winhong.com> > --- > src/util/virconf.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) The patch itself is okay. But can you please to the same change for the whole file and resend? That would be great and render even nicer diff stat :-) Michal
Switch to using the 'g_auto*' helpers.
Signed-off-by: Yi Li <yili@winhong.com>
---
src/util/virconf.c | 47 +++++++++++++++-------------------------------
1 file changed, 15 insertions(+), 32 deletions(-)
diff --git a/src/util/virconf.c b/src/util/virconf.c
index 16107bce96..17fbea2397 100644
--- a/src/util/virconf.c
+++ b/src/util/virconf.c
@@ -573,7 +573,7 @@ static int
virConfParseComment(virConfParserCtxtPtr ctxt)
{
const char *base;
- char *comm;
+ g_autofree char *comm;
if (CUR != '#')
return -1;
@@ -581,10 +581,9 @@ virConfParseComment(virConfParserCtxtPtr ctxt)
base = ctxt->cur;
while ((ctxt->cur < ctxt->end) && (!IS_EOL(CUR))) NEXT;
comm = g_strndup(base, ctxt->cur - base);
- if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL) {
- VIR_FREE(comm);
+ if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL)
return -1;
- }
+
return 0;
}
@@ -626,9 +625,9 @@ static int
virConfParseStatement(virConfParserCtxtPtr ctxt)
{
const char *base;
- char *name;
+ g_autofree char *name;
virConfValuePtr value;
- char *comm = NULL;
+ g_autofree char *comm = NULL;
SKIP_BLANKS_AND_EOL;
if (CUR == '#')
@@ -639,16 +638,13 @@ virConfParseStatement(virConfParserCtxtPtr ctxt)
SKIP_BLANKS;
if (CUR != '=') {
virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("expecting an assignment"));
- VIR_FREE(name);
return -1;
}
NEXT;
SKIP_BLANKS;
value = virConfParseValue(ctxt);
- if (value == NULL) {
- VIR_FREE(name);
+ if (value == NULL)
return -1;
- }
SKIP_BLANKS;
if (CUR == '#') {
NEXT;
@@ -657,9 +653,7 @@ virConfParseStatement(virConfParserCtxtPtr ctxt)
comm = g_strndup(base, ctxt->cur - base);
}
if (virConfAddEntry(ctxt->conf, name, value, comm) == NULL) {
- VIR_FREE(name);
virConfFreeValue(value);
- VIR_FREE(comm);
return -1;
}
return 0;
@@ -724,7 +718,7 @@ virConfParse(const char *filename, const char *content, int len,
virConfPtr
virConfReadFile(const char *filename, unsigned int flags)
{
- char *content;
+ g_autofree char *content;
int len;
virConfPtr conf;
@@ -740,7 +734,6 @@ virConfReadFile(const char *filename, unsigned int flags)
conf = virConfParse(filename, content, len, flags);
- VIR_FREE(content);
return conf;
}
@@ -1412,7 +1405,7 @@ virConfWriteFile(const char *filename, virConfPtr conf)
virConfEntryPtr cur;
int ret;
int fd;
- char *content;
+ g_autofree char *content;
unsigned int use;
if (conf == NULL)
@@ -1433,7 +1426,6 @@ virConfWriteFile(const char *filename, virConfPtr conf)
use = virBufferUse(&buf);
content = virBufferContentAndReset(&buf);
ret = safewrite(fd, content, use);
- VIR_FREE(content);
VIR_FORCE_CLOSE(fd);
if (ret != (int)use) {
virConfError(NULL, VIR_ERR_WRITE_FAILED, _("failed to save content"));
@@ -1461,7 +1453,7 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf)
{
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
virConfEntryPtr cur;
- char *content;
+ g_autofree char *content;
unsigned int use;
if ((memory == NULL) || (len == NULL) || (*len <= 0) || (conf == NULL))
@@ -1478,11 +1470,9 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf)
if ((int)use >= *len) {
*len = (int)use;
- VIR_FREE(content);
return -1;
}
memcpy(memory, content, use);
- VIR_FREE(content);
*len = use;
return use;
}
@@ -1505,26 +1495,19 @@ virConfLoadConfigPath(const char *name)
int
virConfLoadConfig(virConfPtr *conf, const char *name)
{
- char *path = NULL;
- int ret = -1;
+ g_autofree char *path = NULL;
*conf = NULL;
if (!(path = virConfLoadConfigPath(name)))
- goto cleanup;
+ return -1;
- if (!virFileExists(path)) {
- ret = 0;
- goto cleanup;
- }
+ if (!virFileExists(path))
+ return 0;
VIR_DEBUG("Loading config file '%s'", path);
if (!(*conf = virConfReadFile(path, 0)))
- goto cleanup;
-
- ret = 0;
+ return -1;
- cleanup:
- VIR_FREE(path);
- return ret;
+ return 0;
}
--
2.25.3
On 3/12/21 10:51 AM, Yi Li wrote:
> Switch to using the 'g_auto*' helpers.
>
> Signed-off-by: Yi Li <yili@winhong.com>
> ---
> src/util/virconf.c | 47 +++++++++++++++-------------------------------
> 1 file changed, 15 insertions(+), 32 deletions(-)
Almost :-)
>
> diff --git a/src/util/virconf.c b/src/util/virconf.c
> index 16107bce96..17fbea2397 100644
> --- a/src/util/virconf.c
> +++ b/src/util/virconf.c
> @@ -573,7 +573,7 @@ static int
> virConfParseComment(virConfParserCtxtPtr ctxt)
> {
> const char *base;
> - char *comm;
> + g_autofree char *comm;
This leaves @comm uninitialized, therefore comm is assigned "random"
value (whatever was on the stack earlier).
>
> if (CUR != '#')
> return -1;
And thus if this branch is taken, the autofree kicks in (the variable is
going out of scope) and frees random pointer. You can imagine how
dangerous that is.
> @@ -581,10 +581,9 @@ virConfParseComment(virConfParserCtxtPtr ctxt)
> base = ctxt->cur;
> while ((ctxt->cur < ctxt->end) && (!IS_EOL(CUR))) NEXT;
> comm = g_strndup(base, ctxt->cur - base);
> - if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL) {
> - VIR_FREE(comm);
> + if (virConfAddEntry(ctxt->conf, NULL, NULL, comm) == NULL)
> return -1;
> - }
> +
And here, @comm was consumed by virConfAddEntry() - it was added at the
end of ctxt->conf list. Therefore, we must refrain from freeing it here.
Otherwise we leave a pointer behind (at the end of the list) that points
to a memory that was freed. One way to avoid this, is to set comm
explicitly to NULL (with a comment that virConfAddEntry() consumed it),
because then when g_autofree comes and calls free() it does so over a
NULL pointer which is defined to be a NO-OP.
However, I think the cleaner solution is to modify virConfAddEntry() so
that it doesn't take just the first degree pointer, but the second
degree. Because then it's clear to see that ownership of the memory is
transferred - just by looking at the call:
virConfAddEntry(ctxt->conf, NULL, NULL, &comm);
Inside of virConfAddEntry() g_steal_pointer() can be used for assignment
and clearing *comm, like this:
ret->comment = g_steal_pointer(comm);
Of course, this applies transitively to other arguments (unfortunately,
here only NULL is passed, but you get the idea).
BTW: this was caught also by our testsuite (ninja -C _build test) -
virconftest is crashing with this patch.
Michal
© 2016 - 2024 Red Hat, Inc.