[igt-dev] [PATCH i-g-t] lib/igt_core: Don't return too early in common_init_config

[Dixit, Ashutosh]

On Tue, 03 Nov 2020 00:05:13 -0800, Petri Latvala wrote:
>
> On Mon, Nov 02, 2020 at 09:13:25PM -0800, Dixit, Ashutosh wrote:
> > On Mon, 26 Oct 2020 01:48:40 -0700, Petri Latvala wrote:
> > >
> > > common_init_config is responsible for adding the device filters
> > > supplied through the environment to the filter list. If the .igtrc
> > > file cannot be opened, make sure the filter is added still.
> > >
> > > Signed-off-by: Petri Latvala <petri.latvala at intel.com>
> > > Cc: Arkadiusz Hiler <arek at hiler.eu>
> > > ---
> > >  lib/igt_core.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > > index b358173f..8f9d925b 100644
> > > --- a/lib/igt_core.c
> > > +++ b/lib/igt_core.c
> > > @@ -775,21 +775,20 @@ GKeyFile *igt_load_igtrc(void)
> > >  static void common_init_config(void)
> > >  {
> > >	GError *error = NULL;
> > > -	int ret;
> > > +	int ret = 0;
> > >
> > >	igt_key_file = igt_load_igtrc();
> > > -	if (!igt_key_file)
> > > -		return;
> >
> > I believe the only change needed is the one above. The remaining
> > igt_key_file checks below are not needed because it looks like if we pass
> > NULL igt_key_file g_key_file_get_string will return NULL and
> > g_key_file_get_integer will return 0 (that is previously initialized values
> > are undisturbed).
> >
> > At the link below it is not clearly stated what happens when GKeyFile is
> > NULL so I am mostly inferring from the way it is handling other error
> > cases, so if we want to do this we'll probably need to test it out:
>
> If it's not documented to work that way, I'm wary of calling those
> functions with NULL. It could behave differently in different versions
> of glib.

OK, once again:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>

> >
> > https://developer.gnome.org/glib/stable/glib-Key-value-file-parser.html
> >
> > Otherwise the patch is fine:
> >
> > Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> >
> >
> > > -	if (!igt_frame_dump_path)
> > > +	if (igt_key_file && !igt_frame_dump_path)
> > >		igt_frame_dump_path =
> > >			g_key_file_get_string(igt_key_file, "Common",
> > >					      "FrameDumpPath", &error);
> > >
> > >	g_clear_error(&error);
> > >
> > > -	ret = g_key_file_get_integer(igt_key_file, "DUT", "SuspendResumeDelay",
> > > -				     &error);
> > > +	if (igt_key_file)
> > > +		ret = g_key_file_get_integer(igt_key_file, "DUT", "SuspendResumeDelay",
> > > +					     &error);
> > >	assert(!error || error->code != G_KEY_FILE_ERROR_INVALID_VALUE);
> > >
> > >	g_clear_error(&error);
> > > @@ -804,9 +803,10 @@ static void common_init_config(void)
> > >		if (igt_rc_device) {
> > >			igt_debug("Notice: using IGT_DEVICE env:\n");
> > >		} else {
> > > -			igt_rc_device =	g_key_file_get_string(igt_key_file,
> > > -							      "Common",
> > > -							      "Device", &error);
> > > +			if (igt_key_file)
> > > +				igt_rc_device =	g_key_file_get_string(igt_key_file,
> > > +								      "Common",
> > > +								      "Device", &error);
> > >			g_clear_error(&error);
> > >			if (igt_rc_device)
> > >				igt_debug("Notice: using .igtrc "