Re: [XEN PATCH v2 2/5] xen: export get_free_port

From: Jan Beulich <jbeulich@suse.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: jgross@suse.com, Bertrand.Marquis@arm.com, julien@xen.org,
	Volodymyr_Babchuk@epam.com,
	Stefano Stabellini <stefano.stabellini@xilinx.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH v2 2/5] xen: export get_free_port
Date: Fri, 14 Jan 2022 08:02:13 +0100	[thread overview]
Message-ID: <758b304f-4df7-4fa7-fa0a-8ebbebb661d5@suse.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2201131713140.19362@ubuntu-linux-20-04-desktop>

On 14.01.2022 02:20, Stefano Stabellini wrote:
> On Thu, 13 Jan 2022, Jan Beulich wrote:
>> On 13.01.2022 01:58, Stefano Stabellini wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>>>      return 0;
>>>  }
>>>  
>>> -static int get_free_port(struct domain *d)
>>> +int get_free_port(struct domain *d)
>>
>> The name of the function isn't really suitable for being non-static.
>> Can't we fold its functionality back into evtchn_allocate_port() (or
>> the other way around, depending on the perspective you want to take)
>> in case the caller passes in port 0? (Btw., it is imo wrong for the
>> loop over ports to start at 0, when it is part of the ABI that port
>> 0 is always invalid. evtchn_init() also better wouldn't depend on it
>> being the only party to successfully invoke the function getting back
>> port 0.)
> 
> I agree that "get_free_port" is not a great name for a non-static
> function. Also, it should be noted that for the sake of this patch
> series I could just call evtchn_allocate_port(d, 1) given that it is the
> first evtchn to be allocated. So maybe, that's the best way forward?
> 
> 
> To address your specific suggestion, in my opinion it would be best to
> have two different functions to allocate a new port:
> - one with a specific evtchn_port_t port parameter
> - one without it (meaning: "I don't care about the number")
> 
> Folding the functionality of "give me any number" when 0 is passed to
> evtchn_allocate_port() doesn't seem to be an improvement to the API to
> me.

I view it the other way around - that way the function would actually
start matching its name. So far it's more like marking a given port
number as in use, rather than allocating.

> That said, I am still OK with making the suggested change if that's what
> you prefer.

Given experience, hoping for others to voice an opinion isn't likely
to become reality.

Jan