[PATCH i-g-t 1/2] lib/igt_chamelium: check drmModeGetConnector() return value in wait_for_connected_state()

[Luca Coelho]

On Thu, 2024-05-23 at 12:20 +0200, Kamil Konieczny wrote:
> Hi Luca,
> On 2024-05-23 at 11:44:25 +0300, Luca Coelho wrote:
> > The drmModeGetConnector() function can return NULL in some cases, so
> > we need to check the return value before accessing it.  This is not
> > being checked in wait_for_connected_state(), so fix that.
> > 
> > Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> > ---
> >  lib/igt_chamelium.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c
> > index 016d5356630c..b1478654d02a 100644
> > --- a/lib/igt_chamelium.c
> > +++ b/lib/igt_chamelium.c
> > @@ -524,6 +524,9 @@ static bool wait_for_connected_state(int drm_fd,
> >  			drmModeConnector *connector =
> >  				drmModeGetConnector(drm_fd, connectors[i]);
> >  
> > +			if (!connector)
> > +				break;
> 
> Why break? btw please document what is that function
> tring to do, it looks strange.

I did not implement this function, so I'm not the best person to
document what it does, I think...


> Is it:
> wait for one connector to enter connected state
> or
> wait for all connectors to be connected?
> 
> See below.
> 
> > +
> >  			connected = connector->connection == DRM_MODE_CONNECTED;
> >  
> >  			drmModeFreeConnector(connector);
> > -- 
> > 2.39.2
> > 
> 
> Some comments on this function code:
> 
> igt_until_timeout(CHAMELIUM_HOTPLUG_DETECTION_DELAY) {
>     bool connected;
> 
>     for (int i = 0; i < connector_count; i++) {
>         drmModeConnector *connector = drmModeGetConnector(drm_fd, connectors[i]);
> 
> 		if (!connector)
> 				continue;
> 
>         connected = connector->connection == DRM_MODE_CONNECTED;
>         drmModeFreeConnector(connector);
> 
>         if (!connected)
> ------------^
>             break;
> ------------^
> Why break here? imho this should be:

It is the same thing that happens if the connector is not connected. 
It will leave the inner loop and continue with the outer, timeout loop.


>         if (connected)
>             return true;
> 
>         }
> 
>     if (connected)
> ------------^
>         return true;
> 
> This if(..)... could be then deleted.

The goal of this function seems to be to wait until all connectors are
connected.  As it is, we will only get to this place with connected ==
true if all the connectors are connected.  If we return true when the
first one is connected, we're changing the semantics of this function.

--
Cheers,
Luca.