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

[Jiang Jiacheng <jiangjiacheng@xxxxxxxxxx>]

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@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.
> 

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;