Re: [PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.07.2022 17:22, Julien Grall wrote:
> Hi Jan,
> 
> On 05/07/2022 16:11, Jan Beulich wrote:
>> On 22.06.2022 16:38, Rahul Singh wrote:
>>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t 
>>> *bind)
>>>           spin_lock(&ld->event_lock);
>>>       }
>>>   
>>> -    if ( (lport = get_free_port(ld)) < 0 )
>>> -        ERROR_EXIT(lport);
>>> +    if ( lport != 0 )
>>> +    {
>>> +        if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
>>> +            ERROR_EXIT_DOM(rc, ld);
>>> +    }
>>> +    else
>>> +    {
>>> +        int alloc_port = get_free_port(ld);
>>> +
>>> +        if ( alloc_port < 0 )
>>> +            ERROR_EXIT_DOM(alloc_port, ld);
>>> +        lport = alloc_port;
>>> +    }
>>
>> This is then the 3rd instance of this pattern. I think the logic
>> wants moving into get_free_port() (perhaps with a name change).
> 
> I think the code below would be suitable. I can send it or Rahul can 
> re-use the commit [1]:

Ah yes; probably makes sense (right now) only in the context of his
series.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>       xsm_evtchn_close_post(chn);
>   }
> 
> +static int evtchn_get_port(struct domain *d, uint32_t port)

Preferably evtchn_port_t.

> +{
> +    int rc;
> +
> +    if ( port != 0 )
> +        rc = evtchn_allocate_port(d, port);
> +    else
> +        rc = get_free_port(d);
> +
> +    return rc != 0 ? rc : port;

We commonly use "rc ?: port" in such cases. At which point I'd be
inclined to make it just

static int evtchn_get_port(struct domain *d, evtchn_port_t port)
{
    return (port ? evtchn_allocate_port(d, port)
                 : get_free_port(d)) ?: port;
}

But I can see you or others having reservations against such ...

Jan