Re: [PATCH 1/2] xen: add domid_to_domain() helper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2022 10:36, Julien Grall wrote:
> On 12/09/2022 09:33, Juergen Gross wrote:
>> On 12.09.22 10:31, Jan Beulich wrote:
>>> On 12.09.2022 10:23, Juergen Gross wrote:
>>>> On 12.09.22 10:19, Jan Beulich wrote:
>>>>> On 12.09.2022 07:53, Juergen Gross wrote:
>>>>>> Add a helper domid_to_domain() returning the struct domain pointer for
>>>>>> a domain give by its domid and which is known not being able to be
>>>>>> released (its reference count isn't incremented and no 
>>>>>> rcu_lock_domain()
>>>>>> is called for it).
>>>>>>
>>>>>> In order to simplify coding add an internal helper for doing the 
>>>>>> lookup
>>>>>> and call that from the new function and similar functions.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
>>>>>
>>>>> I don't see an issue with adding such a helper (responding to your 
>>>>> concern
>>>>> in the cover letter), but I think the constraints need to be empahsized
>>>>> more: We already have get_knownalive_domain() and 
>>>>> get_domain_by_id(), so
>>>>> how about naming the new helper get_knownalive_domain_by_id()? And 
>>>>> then ...
>>>>
>>>> I explicitly didn't name it "get_...", as those helpers all increment 
>>>> the
>>>> reference count of the domain. And this is NOT done by the new helper.
>>>
>>> Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" 
>>> aspect,
>>> yet that's relevant to see when reviewing new uses of the function. 
>>> Such uses
>>> aren't likely to make the reviewer go look at the function declaration 
>>> when
>>> the function name is pretty "innocent".
>>
>> Okay, what about domid_to_knownalive_domain()?
>>
>> Or knownalive_domain_from_domid()?
> 
> FWIW, I am fine with either. However, please don't replace 'to' with '2' 
> if you need a internal helper. Just suffixed with 'locked' to make clear 
> this is a version where the caller should take the lock.

Hmm - personally I dislike "_locked" suffixes on functions. If the
"internal helper" aspect is to be made more explicit, then perhaps
by way of prefixing a single underscore?

Jan