Re: [PATCH 2/3] src/tests: use g_autoptr for virNWFilterDef and virNWFilterRuleDef

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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;

Attachment: signature.asc
Description: PGP signature