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

[Jan Beulich <jbeulich@xxxxxxxx>]

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