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

[Julien Grall <julien@xxxxxxx>]

On 12/09/2022 09:42, Jan Beulich wrote:
> 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?
I am OK with that. Although, from previous discussion, I would have 
expected that explicit name would have been your preference ("locked" is 
more explicit than "_").
Cheers,

--
Julien Grall