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