[libvirt] [PATCH 1.5/3] conf: use VIR_AUTOPTR as much as possible for virNetworkPortDefPtr

[Laine Stump]

On 9/26/19 2:52 AM, Michal Privoznik wrote:
> On 9/23/19 3:08 AM, Laine Stump wrote:
>> Since the VIR_DEFINE_AUTOPTR_FUNC() was added for
>> virNetworkPortDefPtr, I decided to convert all uses of
>> virNetworkPortDefPtr that were appropriate to use VIR_AUTOPTR. This 
>> could be
>> squashed into patch 1/2, or left separate, or just completely dropped.
>>
>> Signed-off-by: Laine Stump <laine at redhat.com>
>> ---
>>   src/conf/domain_conf.c            | 58 ++++++++++++++-----------------
>>   src/conf/virnetworkobj.c          |  3 +-
>>   src/conf/virnetworkportdef.c      | 52 +++++++++++++--------------
>>   tests/virnetworkportxml2xmltest.c |  3 +-
>>   4 files changed, 53 insertions(+), 63 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index d1e7ac84e8..d638c455d0 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -30565,7 +30565,8 @@ virNetworkPortDefPtr
>>   virDomainNetDefToNetworkPort(virDomainDefPtr dom,
>>                                virDomainNetDefPtr iface)
>>   {
>> -    virNetworkPortDefPtr port;
>> +    VIR_AUTOPTR(virNetworkPortDef) port = NULL;
>> +    virNetworkPortDefPtr portret = NULL;
>
> Here and in the rest of the patch you don need to introduce XXXret 
> variable, because ...
>
>
>> -    return port;
>> -
>> - error:
>> -    virNetworkPortDefFree(port);
>> -    return NULL;
>> +    VIR_STEAL_PTR(portret, port);
>> +    return portret;
>
> .. you can just use VIR_RETURN_PTR(port);


Ah, I never noticed that macro. Exactly what I wanted :-)


>
>>   }
>
>
> Also, there is one more occurrence of virNetworkPortDefFree() in 
> networkPortCreateXML() in src/network/bridge_driver.c. In fact, code 
> inspection says that virNetworkPortDef might be leaked there (for 
> instance if checks involving @portdef in the middle of the function 
> fail), so please squash this in:
>
> diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c
> index c54be96407..f9ef7eeb6f 100644
> --- i/src/network/bridge_driver.c
> +++ w/src/network/bridge_driver.c
> @@ -5571,7 +5571,7 @@ networkPortCreateXML(virNetworkPtr net,
>      virNetworkDriverStatePtr driver = networkGetDriver();
>      virNetworkObjPtr obj;
>      virNetworkDefPtr def;
> -    virNetworkPortDefPtr portdef = NULL;
> +    VIR_AUTOPTR(virNetworkPortDef) portdef = NULL;
>      virNetworkPortPtr ret = NULL;
>      int rc;
>
> @@ -5621,13 +5621,13 @@ networkPortCreateXML(virNetworkPtr net,
>
>          virErrorPreserveLast(&save_err);
>          ignore_value(networkReleasePort(obj, portdef));
> -        virNetworkPortDefFree(portdef);
>          virErrorRestore(&save_err);
>
>          goto cleanup;
>      }
>
>      ret = virGetNetworkPort(net, portdef->uuid);
> +    portdef = NULL;
>   cleanup:
>      virNetworkObjEndAPI(&obj);
>      return ret;


I had looked at that one (I used cscope to look at all of them, and for 
some reason had decided it was inappropriate to convert. But your 
suggestion shows that I was just looking at it wrong. So yes, that looks 
like a good idea.


I'll make the changes before pushing.


Thanks!