5:19 a.m.
Define and use g_autoptr() for virNWFilterDef and virNWFilterRuleDef,
and remove unnecessary label/variable.
Those patches are followup with:
https://gitlab.com/libvirt/libvirt/-/commit/a9027d447be789cd11c0aa18ceb83...
Jiang Jiacheng (3):
conf: define g_autoptr for virNWFilterDef and virNWFilterRuleDef
src/tests: use g_autoptr for virNWFilterDef and virNWFilterRuleDef
virNWFilterSaveConfig: remove the unnecessary variable 'ret'
src/conf/nwfilter_conf.c | 52 ++++++++++++++--------------------
src/conf/nwfilter_conf.h | 2 ++
src/conf/virnwfilterobj.c | 19 ++++++-------
src/nwfilter/nwfilter_driver.c | 7 ++---
tests/nwfilterxml2xmltest.c | 22 ++++++--------
5 files changed, 42 insertions(+), 60 deletions(-)
--
2.33.0
5:19 a.m.
New subject: [PATCH 1/3] conf: define g_autoptr for virNWFilterDef and virNWFilterRuleDef
Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/nwfilter_conf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index 1a38e4198c..7c09b3bcb9 100644
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -522,9 +522,11 @@ struct _virNWFilterDef {
void
virNWFilterRuleDefFree(virNWFilterRuleDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNWFilterRuleDef, virNWFilterRuleDefFree);
void
virNWFilterDefFree(virNWFilterDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNWFilterDef, virNWFilterDefFree);
int
virNWFilterTriggerRebuild(void);
--
2.33.0
5:19 a.m.
New subject: [PATCH 2/3] src/tests: use g_autoptr for virNWFilterDef and virNWFilterRuleDef
Use g_autoptr() for virNWFilterDef and virNWFilterRuleDef and remove
unnecessary label.
Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/nwfilter_conf.c | 44 ++++++++++++++--------------------
src/conf/virnwfilterobj.c | 19 +++++++--------
src/nwfilter/nwfilter_driver.c | 7 +++---
tests/nwfilterxml2xmltest.c | 22 +++++++----------
4 files changed, 37 insertions(+), 55 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 5453b6db05..382dafb13b 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2384,7 +2384,7 @@ virNWFilterRuleParse(xmlNodePtr node)
int priority;
xmlNodePtr cur;
- virNWFilterRuleDef *ret;
+ g_autoptr(virNWFilterRuleDef) ret = NULL;
ret = g_new0(virNWFilterRuleDef, 1);
@@ -2397,28 +2397,28 @@ virNWFilterRuleParse(xmlNodePtr node)
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s",
_("rule node requires action attribute"));
- goto err_exit;
+ return NULL;
}
if ((ret->action = virNWFilterRuleActionTypeFromString(action)) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
_("unknown rule action attribute value"));
- goto err_exit;
+ return NULL;
}
if (!direction) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s",
_("rule node requires direction attribute"));
- goto err_exit;
+ return NULL;
}
if ((ret->tt = virNWFilterRuleDirectionTypeFromString(direction)) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s",
_("unknown rule direction attribute value"));
- goto err_exit;
+ return NULL;
}
ret->priority = MAX_RULE_PRIORITY / 2;
@@ -2455,10 +2455,10 @@ virNWFilterRuleParse(xmlNodePtr node)
if (virNWFilterRuleDetailsParse(cur,
ret,
virAttr[i].att) < 0) {
- goto err_exit;
+ return NULL;
}
if (virNWFilterRuleValidate(ret) < 0)
- goto err_exit;
+ return NULL;
break;
}
if (!found) {
@@ -2476,11 +2476,7 @@ virNWFilterRuleParse(xmlNodePtr node)
virNWFilterRuleDefFixup(ret);
- return ret;
-
- err_exit:
- g_clear_pointer(&ret, virNWFilterRuleDefFree);
- return NULL;
+ return g_steal_pointer(&ret);
}
@@ -2561,7 +2557,7 @@ virNWFilterIsAllowedChain(const char *chainname)
static virNWFilterDef *
virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
{
- virNWFilterDef *ret;
+ g_autoptr(virNWFilterDef) ret = NULL;
xmlNodePtr curr = ctxt->node;
g_autofree char *uuid = NULL;
g_autofree char *chain = NULL;
@@ -2576,7 +2572,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
if (!ret->name) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("filter has no name"));
- goto cleanup;
+ return NULL;
}
chain_pri_s = virXPathString("string(./@priority)", ctxt);
@@ -2585,7 +2581,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
virReportError(VIR_ERR_INVALID_ARG,
_("Could not parse chain priority '%s'"),
chain_pri_s);
- goto cleanup;
+ return NULL;
}
if (chain_priority < NWFILTER_MIN_FILTER_PRIORITY ||
chain_priority > NWFILTER_MAX_FILTER_PRIORITY) {
@@ -2595,7 +2591,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
chain_priority,
NWFILTER_MIN_FILTER_PRIORITY,
NWFILTER_MAX_FILTER_PRIORITY);
- goto cleanup;
+ return NULL;
}
}
@@ -2603,7 +2599,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
if (chain) {
name_prefix = virNWFilterIsAllowedChain(chain);
if (name_prefix == NULL)
- goto cleanup;
+ return NULL;
ret->chainsuffix = g_steal_pointer(&chain);
if (chain_pri_s) {
@@ -2626,13 +2622,13 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
if (virUUIDGenerate(ret->uuid) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("unable to generate uuid"));
- goto cleanup;
+ return NULL;
}
} else {
if (virUUIDParse(uuid, ret->uuid) < 0) {
virReportError(VIR_ERR_XML_ERROR,
"%s", _("malformed uuid element"));
- goto cleanup;
+ return NULL;
}
}
@@ -2645,12 +2641,12 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
if (virXMLNodeNameEqual(curr, "rule")) {
if (!(entry->rule = virNWFilterRuleParse(curr))) {
virNWFilterEntryFree(entry);
- goto cleanup;
+ return NULL;
}
} else if (virXMLNodeNameEqual(curr, "filterref")) {
if (!(entry->include = virNWFilterIncludeParse(curr))) {
virNWFilterEntryFree(entry);
- goto cleanup;
+ return NULL;
}
}
@@ -2663,11 +2659,7 @@ virNWFilterDefParseXML(xmlXPathContextPtr ctxt)
curr = curr->next;
}
- return ret;
-
- cleanup:
- virNWFilterDefFree(ret);
- return NULL;
+ return g_steal_pointer(&ret);
}
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 2e75e90cf1..2a2d877f49 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -564,37 +564,34 @@ virNWFilterObjListLoadConfig(virNWFilterObjList *nwfilters,
const char *configDir,
const char *name)
{
- virNWFilterDef *def = NULL;
+ g_autoptr(virNWFilterDef) def = NULL;
virNWFilterObj *obj;
g_autofree char *configFile = NULL;
if (!(configFile = virFileBuildPath(configDir, name, ".xml")))
- goto error;
+ return NULL;
if (!(def = virNWFilterDefParse(NULL, configFile, 0)))
- goto error;
+ return NULL;
if (STRNEQ(name, def->name)) {
virReportError(VIR_ERR_XML_ERROR,
_("network filter config filename '%s' "
"does not match name '%s'"),
configFile, def->name);
- goto error;
+ return NULL;
}
/* We generated a UUID, make it permanent by saving the config to disk */
if (!def->uuid_specified &&
virNWFilterSaveConfig(configDir, def) < 0)
- goto error;
+ return NULL;
- if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
- goto error;
+ if (!(obj = virNWFilterObjListAssignDef(nwfilters,
+ g_steal_pointer(&def))))
+ return NULL;
return obj;
-
- error:
- virNWFilterDefFree(def);
- return NULL;
}
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 8e45096eaa..be21aa12c2 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -531,7 +531,7 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
unsigned int flags)
{
VIR_LOCK_GUARD lock = virLockGuardLock(&driverMutex);
- virNWFilterDef *def;
+ g_autoptr(virNWFilterDef) def = NULL;
virNWFilterObj *obj = NULL;
virNWFilterDef *objdef;
virNWFilterPtr nwfilter = NULL;
@@ -552,10 +552,10 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
goto cleanup;
VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
- if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def)))
+ if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters,
+ g_steal_pointer(&def))))
goto cleanup;
}
- def = NULL;
objdef = virNWFilterObjGetDef(obj);
if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
@@ -566,7 +566,6 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
nwfilter = virGetNWFilter(conn, objdef->name, objdef->uuid);
cleanup:
- virNWFilterDefFree(def);
if (obj)
virNWFilterObjUnlock(obj);
return nwfilter;
diff --git a/tests/nwfilterxml2xmltest.c b/tests/nwfilterxml2xmltest.c
index ca037ad9a0..5c84c2fee9 100644
--- a/tests/nwfilterxml2xmltest.c
+++ b/tests/nwfilterxml2xmltest.c
@@ -16,31 +16,25 @@ testCompareXMLToXMLFiles(const char *inxml, const char *outxml,
bool expect_error)
{
g_autofree char *actual = NULL;
- int ret = -1;
- virNWFilterDef *dev = NULL;
+ g_autoptr(virNWFilterDef) def = NULL;
virResetLastError();
- if (!(dev = virNWFilterDefParse(NULL, inxml, 0))) {
+ if (!(def = virNWFilterDefParse(NULL, inxml, 0))) {
if (expect_error) {
virResetLastError();
- goto done;
+ return 0;
}
- goto fail;
+ return -1;
}
- if (!(actual = virNWFilterDefFormat(dev)))
- goto fail;
+ if (!(actual = virNWFilterDefFormat(def)))
+ return -1;
if (virTestCompareToFile(actual, outxml) < 0)
- goto fail;
+ return -1;
- done:
- ret = 0;
-
- fail:
- virNWFilterDefFree(dev);
- return ret;
+ return 0;
}
typedef struct test_parms {
--
2.33.0
9 a.m.
New subject: [PATCH 2/3] src/tests: use g_autoptr for virNWFilterDef and virNWFilterRuleDef
On a Monday in 2023, Jiang Jiacheng wrote:
Use g_autoptr() for virNWFilterDef and virNWFilterRuleDef and remove
unnecessary label.
Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/nwfilter_conf.c | 44 ++++++++++++++--------------------
src/conf/virnwfilterobj.c | 19 +++++++--------
src/nwfilter/nwfilter_driver.c | 7 +++---
tests/nwfilterxml2xmltest.c | 22 +++++++----------
4 files changed, 37 insertions(+), 55 deletions(-)
diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
index 2e75e90cf1..2a2d877f49 100644
--- a/src/conf/virnwfilterobj.c
+++ b/src/conf/virnwfilterobj.c
@@ -564,37 +564,34 @@ virNWFilterObjListLoadConfig(virNWFilterObjList *nwfilters,
const char *configDir,
const char *name)
{
- virNWFilterDef *def = NULL;
+ g_autoptr(virNWFilterDef) def = NULL;
virNWFilterObj *obj;
g_autofree char *configFile = NULL;
if (!(configFile = virFileBuildPath(configDir, name, ".xml")))
- goto error;
+ return NULL;
if (!(def = virNWFilterDefParse(NULL, configFile, 0)))
- goto error;
+ return NULL;
if (STRNEQ(name, def->name)) {
virReportError(VIR_ERR_XML_ERROR,
_("network filter config filename '%s' "
"does not match name '%s'"),
configFile, def->name);
- goto error;
+ return NULL;
}
/* We generated a UUID, make it permanent by saving the config to disk */
if (!def->uuid_specified &&
virNWFilterSaveConfig(configDir, def) < 0)
- goto error;
+ return NULL;
- if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
- goto error;
+ if (!(obj = virNWFilterObjListAssignDef(nwfilters,
+ g_steal_pointer(&def))))
+ return NULL;
Stealing the pointer here would lead to a memory leak if
virNWFilterObjListAssignDef fails.
The pointer can only be set to NULL after it was successfully assigned
to the definition.
return obj;
-
- error:
- virNWFilterDefFree(def);
- return NULL;
}
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 8e45096eaa..be21aa12c2 100644
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -531,7 +531,7 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
@@ -552,10 +552,10 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
goto cleanup;
VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
- if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def)))
+ if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters,
+ g_steal_pointer(&def))))
goto cleanup;
}
- def = NULL;
Same here. These two changes can be dropped.
Jano
objdef = virNWFilterObjGetDef(obj);
if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
@@ -566,7 +566,6 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
nwfilter = virGetNWFilter(conn, objdef->name, objdef->uuid);
cleanup:
- virNWFilterDefFree(def);
if (obj)
virNWFilterObjUnlock(obj);
return nwfilter;
5:31 a.m.
New subject: [PATCH 2/3] src/tests: use g_autoptr for virNWFilterDef and virNWFilterRuleDef
On 2023/1/9 23:00, Ján Tomko wrote:
On a Monday in 2023, Jiang Jiacheng wrote:
> Use g_autoptr() for virNWFilterDef and virNWFilterRuleDef and remove
> unnecessary label.
>
> Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com>
> ---
> src/conf/nwfilter_conf.c | 44 ++++++++++++++--------------------
> src/conf/virnwfilterobj.c | 19 +++++++--------
> src/nwfilter/nwfilter_driver.c | 7 +++---
> tests/nwfilterxml2xmltest.c | 22 +++++++----------
> 4 files changed, 37 insertions(+), 55 deletions(-)
>
> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
> index 2e75e90cf1..2a2d877f49 100644
> --- a/src/conf/virnwfilterobj.c
> +++ b/src/conf/virnwfilterobj.c
> @@ -564,37 +564,34 @@ virNWFilterObjListLoadConfig(virNWFilterObjList
> *nwfilters,
> const char *configDir,
> const char *name)
> {
> - virNWFilterDef *def = NULL;
> + g_autoptr(virNWFilterDef) def = NULL;
> virNWFilterObj *obj;
> g_autofree char *configFile = NULL;
>
> if (!(configFile = virFileBuildPath(configDir, name, ".xml")))
> - goto error;
> + return NULL;
>
> if (!(def = virNWFilterDefParse(NULL, configFile, 0)))
> - goto error;
> + return NULL;
>
> if (STRNEQ(name, def->name)) {
> virReportError(VIR_ERR_XML_ERROR,
> _("network filter config filename '%s' "
> "does not match name '%s'"),
> configFile, def->name);
> - goto error;
> + return NULL;
> }
>
> /* We generated a UUID, make it permanent by saving the config to
> disk */
> if (!def->uuid_specified &&
> virNWFilterSaveConfig(configDir, def) < 0)
> - goto error;
> + return NULL;
>
> - if (!(obj = virNWFilterObjListAssignDef(nwfilters, def)))
> - goto error;
> + if (!(obj = virNWFilterObjListAssignDef(nwfilters,
> + g_steal_pointer(&def))))
> + return NULL;
Stealing the pointer here would lead to a memory leak if
virNWFilterObjListAssignDef fails.
The pointer can only be set to NULL after it was successfully assigned
to the definition.
Thanks for your reply!
I will drop the two changes in the next version.
Thanks
Jiang Jiacheng
>
> return obj;
> -
> - error:
> - virNWFilterDefFree(def);
> - return NULL;
> }
>
>
> diff --git a/src/nwfilter/nwfilter_driver.c
> b/src/nwfilter/nwfilter_driver.c
> index 8e45096eaa..be21aa12c2 100644
> --- a/src/nwfilter/nwfilter_driver.c
> +++ b/src/nwfilter/nwfilter_driver.c
> @@ -531,7 +531,7 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
> @@ -552,10 +552,10 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
> goto cleanup;
>
> VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
> - if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters,
> def)))
> + if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters,
> + g_steal_pointer(&def))))
> goto cleanup;
> }
> - def = NULL;
Same here. These two changes can be dropped.
Jano
> objdef = virNWFilterObjGetDef(obj);
>
> if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
> @@ -566,7 +566,6 @@ nwfilterDefineXMLFlags(virConnectPtr conn,
> nwfilter = virGetNWFilter(conn, objdef->name, objdef->uuid);
>
> cleanup:
> - virNWFilterDefFree(def);
> if (obj)
> virNWFilterObjUnlock(obj);
> return nwfilter;
5:19 a.m.
New subject: [PATCH 3/3] virNWFilterSaveConfig: remove the unnecessary variable 'ret'
Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/nwfilter_conf.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 382dafb13b..8d198a7bb1 100644
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2684,7 +2684,6 @@ int
virNWFilterSaveConfig(const char *configDir,
virNWFilterDef *def)
{
- int ret = -1;
g_autofree char *xml = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
g_autofree char *configFile = NULL;
@@ -2696,11 +2695,10 @@ virNWFilterSaveConfig(const char *configDir,
return -1;
virUUIDFormat(def->uuid, uuidstr);
- ret = virXMLSaveFile(configFile,
- virXMLPickShellSafeComment(def->name, uuidstr),
- "nwfilter-edit", xml);
- return ret;
+ return virXMLSaveFile(configFile,
+ virXMLPickShellSafeComment(def->name, uuidstr),
+ "nwfilter-edit", xml);
}
--
2.33.0
9:04 a.m.
New subject: [PATCH 3/3] virNWFilterSaveConfig: remove the unnecessary variable 'ret'
On a Monday in 2023, Jiang Jiacheng wrote:
Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com>
---
src/conf/nwfilter_conf.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
and pushed now.
Jano
682
days inactive
683
days old
6 comments
2 participants
participants (2)
-
Jiang Jiacheng -
Ján Tomko