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

[Petri Latvala]

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.



-- 
Petri Latvala


> 
> 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 "