Re: [PATCH 3/3] xen/evtchn: Clean up teardown handling

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 3/3] xen/evtchn: Clean up teardown handling
Date: Tue, 22 Dec 2020 13:33:02 +0000	[thread overview]
Message-ID: <91ec88c5-fa7b-e700-2466-322dd3db7397@citrix.com> (raw)
In-Reply-To: <5d66a8c9-e3d6-e329-7daf-6b1d0e220e13@suse.com>

On 22/12/2020 11:52, Jan Beulich wrote:
> On 22.12.2020 12:28, Andrew Cooper wrote:
>> On 22/12/2020 10:48, Jan Beulich wrote:
>>> On 21.12.2020 19:14, Andrew Cooper wrote:
>>>> First of all, rename the evtchn APIs:
>>>>  * evtchn_destroy       => evtchn_teardown
>>>>  * evtchn_destroy_final => evtchn_destroy
>>> I wonder in how far this is going to cause confusion with backports
>>> down the road. May I suggest to do only the first of the two renames,
>>> at least until in a couple of year's time? Or make the second rename
>>> to e.g. evtchn_cleanup() or evtchn_deinit()?
>> I considered backports, but I don't think it will be an issue.  The
>> contents of the two functions are very different, and we're not likely
>> to be moving the callers in backports.
> Does the same also apply to the old and new call sites of the functions?

I don't understand your question.  I don't intend the new callsites to
ever move again, now they're part of the properly idempotent path, and
any movement in the older trees would be wrong for anything other than
backporting this fix, which clearly isn't a backport candidate.

(That said - there's a memory leak I need to create a backport for...)

>> I'm not fussed about the exact naming, so long as we can make and
>> agreement and adhere to it strictly.  The current APIs are a total mess.
>>
>> I used teardown/destroy because that seems to be one common theme in the
>> APIs, but it will require some to change their name.
> So for domains "teardown" and "destroy" pair up with "create". I don't
> think evtchn_create() is a sensible name (the function doesn't really
> "create" anything); evtchn_init() seems quite a bit better to me, and
> hence evtchn_deinit() could be its counterpart.

You're never going to find a perfect name for all cases, and in this
proposal, you've still got evtchn_init/deinit/destroy() as a triple.

What we do need is some clear rules, which will live in the forthcoming
"lifecycle of a domain" document.

> In particular I don't
> think all smaller entity functions involved in doing "xyz" for a
> larger entity need to have "xyz" in their names.

While in principle I agree, I'm not sure the value if having perfect
names outweighs the value of having a simple set of guidelines.

~Andrew