[PATCH 0/3] define g_autoptr for virNWFilterDef and virNWFilterRuleDef
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/a9027d447be789cd11c0aa18ceb83ed7... 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
Signed-off-by: Jiang Jiacheng <jiangjiacheng@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
Use g_autoptr() for virNWFilterDef and virNWFilterRuleDef and remove unnecessary label. Signed-off-by: Jiang Jiacheng <jiangjiacheng@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
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@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;
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@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;
Signed-off-by: Jiang Jiacheng <jiangjiacheng@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
On a Monday in 2023, Jiang Jiacheng wrote:
Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com> --- src/conf/nwfilter_conf.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> and pushed now. Jano
participants (2)
-
Jiang Jiacheng -
Ján Tomko