Re: [PATCH 4/4] src: Use g_steal_pointer() more
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(a)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(a)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