Re: [PATCH] tools/libs/evtchn: drop assert()s in stubdom

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Dec 6, 2023 at 11:44 AM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> On 06.12.23 17:38, Jason Andryuk wrote:
> > On Wed, Dec 6, 2023 at 9:40 AM Juergen Gross <jgross@xxxxxxxx> wrote:
> >>
> >> In tools/libs/evtchn/minios.c there are assert()s for the current
> >> thread being the main thread when binding an event channel.
> >>
> >> As Mini-OS is supporting multiple threads, there is no real reason
> >> why the binding shouldn't be allowed to happen in any other thread.
> >>
> >> Just drop the assert()s.
> >>
> >> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> >> ---
> >>   tools/libs/evtchn/minios.c | 3 ---
> >>   1 file changed, 3 deletions(-)
> >>
> >> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> >> index 28743cb055..e33ddec7e7 100644
> >> --- a/tools/libs/evtchn/minios.c
> >> +++ b/tools/libs/evtchn/minios.c
> >> @@ -195,7 +195,6 @@ xenevtchn_port_or_error_t 
> >> xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
> >>       int ret;
> >>       evtchn_port_t port;
> >>
> >> -    assert(get_current() == main_thread);
> >>       port_info = port_alloc(xce);
> >
> > If multiple threads are allowed, does port_list need to gain a lock
> > protecting it?
>
> I thought of that, too.
>
> The answer is: maybe
>
> Any other list operation on the list isn't protected by an assert(), so
> technically there is no real new aspect added in this regard.

Yes.

> I believe adding a lock would make sense, but it is orthogonal to this
> patch.

The assert() feels like it was an attempt to avoid introducing
locking, so I'm not sure it is really orthogonal.

I was kinda waiting to see if anyone else would lend an opinion.

Since the asserts haven't been tripping there doesn't seem to be an
issue with the code as-is, so:

Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Regards,
Jason