[PATCH 4/4] src: Use g_steal_pointer() more

[Michal Prívozník]

On 2/1/22 17:59, Erik Skultety wrote:
> On Tue, Feb 01, 2022 at 05:23:52PM +0100, Michal Prívozník wrote:
>> On 2/1/22 17:18, Erik Skultety wrote:
>>> On Mon, Jan 31, 2022 at 03:53:42PM +0100, Michal Privoznik wrote:
>>>> There are few places where the g_steal_pointer() is open coded.
>>>> Switch them to calling the g_steal_pointer() function instead.
>>>> Generated by the following spatch:
>>>>
>>>>   @ rule1 @
>>>>   expression a, b;
>>>>   @@
>>>>     <...
>>>>   - b = a;
>>>>     ... when != b
>>>>   - a = NULL;
>>>>   + b = g_steal_pointer(&a);
>>>>     ...>
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>> ...
>>>
>>>> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
>>>> index 7b684e04ba..52851ac507 100644
>>>> --- a/src/hyperv/hyperv_driver.c
>>>> +++ b/src/hyperv/hyperv_driver.c
>>>> @@ -1780,8 +1780,7 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>>  
>>>>      priv->version = g_strdup(os->data->Version);
>>>>  
>>>> -    conn->privateData = priv;
>>>> -    priv = NULL;
>>>> +    conn->privateData = g_steal_pointer(&priv);
>>>>      result = VIR_DRV_OPEN_SUCCESS;
>>>>  
>>>>   cleanup:
>>>> @@ -3518,9 +3517,8 @@ hypervConnectListAllDomains(virConnectPtr conn,
>>>>          doms[count++] = domain;
>>>>      }
>>>>  
>>>> -    if (doms)
>>>> -        *domains = doms;
>>>> -    doms = NULL;
>>>> +    if (domains)
>>>> +        *domains = g_steal_pointer(&doms);
>>>
>>> ^this is not semantically identical, you need to fix it manually before pushing
>>>
>>> Reviewed-by: Erik Skultety <eskultet at redhat.com>
>>>
>>
>> Yeah, hyperv code doesn't get as much love as other drivers, but
>> basically, @domains is allocated iff @doms != NULL, there's the
>> following code earlier in the function:
>>
>>     if (domains) {
>>         doms = g_new0(virDomainPtr, 1);
>>         ndoms = 1;
>>     }
>>
>> I find the new version easier to read, since @domains is an argument of
>> the function. Do you want me to post a separate patch that does
> 
> Well, it's not consistent with the rest which is not good for commit history
> in case this introduced a bug for some reason...

It's not, but I think it's trivial enough to be contained in this
commit. But alright, let me keep the original condition and push.

> 
>> s/doms/domains/?
> 
> It would probably need to a tiny bit more than that, e.g.:
> 
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 7b684e04ba..af71eef7e2 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -3423,7 +3423,6 @@ hypervConnectListAllDomains(virConnectPtr conn,
>      g_autoptr(Msvm_ComputerSystem) computerSystemList = NULL;
>      Msvm_ComputerSystem *computerSystem = NULL;
>      size_t ndoms;
> -    virDomainPtr domain;
>      virDomainPtr *doms = NULL;
>      int count = 0;
>      int ret = -1;
> @@ -3475,6 +3474,7 @@ hypervConnectListAllDomains(virConnectPtr conn,
> 
>      for (computerSystem = computerSystemList; computerSystem != NULL;
>           computerSystem = computerSystem->next) {
> +        virDomainPtr domain = NULL;
> 
>          /* filter by domain state */
>          if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE)) {
> @@ -3509,13 +3509,11 @@ hypervConnectListAllDomains(virConnectPtr conn,
> 
>          VIR_RESIZE_N(doms, ndoms, count, 2);
> 
> -        domain = NULL;
> -
>          if (hypervMsvmComputerSystemToDomain(conn, computerSystem,
>                                               &domain) < 0)
>              goto cleanup;
> 
> -        doms[count++] = domain;
> +        doms[count++] = g_steal_pointer(domain);
>      }
> 
>      if (doms)

This is unrelated. Worth pushing, but unrelated. Note, there are three
variables: doms, domain, and domains. My patch touches only the first
and the last one.

Michal