Hi Jan
> On 5 Jul 2022, at 4:42 pm, Jan Beulich <jbeul...@suse.com> wrote:
>
> 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.
Ack.
>
>> --- 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.
Ack.
>
>> +{
>> + 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 ...
If everyone agree with above code I will modify the Julien patch to include
this code in next version.
Regards,
Rahul
