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

[Erik Skultety]

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

> 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)


Erik