Re: [PATCH 1/3] android: Implement lightweight GKeyFile INI string parser in glib shim

Hi Louis,
On 2026-07-17 at 21:23:17 +0200, Louis Chauvet wrote:
> 
> 
> On 7/17/26 12:18, Kamil Konieczny wrote:
> > Hi Louis,
> > On 2026-07-17 at 10:20:30 +0200, Louis Chauvet wrote:
> > > 
> > > 
> > > On 7/15/26 21:29, Mark Yacoub wrote:
> > > > [Why]
> > > > Android's Bionic C library lacks libglib2, requiring IGT to build against
> > > > an Android-specific mock header (include/android/glib.h). The existing mock
> > > > implementation of GKeyFile and g_key_file_load_from_file is a hardcoded
> > > > no-op returning NULL, which prevents tests relying on configuration files
> > > > (such as .igtrc) from dynamically retrieving values on Android.
> > > 
> > > Hello Mark,
> > > 
> > > I don't think it's a good idea to keep a custom parser inside IGT that isn't
> > > always used, especially when it behaves differently (no character escaping,
> > > first key value is used versus the last, 1/true/yes versus 1/true for
> > > booleans, spaces around =, etc.).
> > > 
> > > I agree that external dependencies can be a pain to manage and are sometimes
> > > unavailable. After a quick look, extracting the full GLib parser doesn't
> > > seem feasible (it's tightly coupled to GLib internals). However, IGT could
> > > easily implement a minimal subset, as you have already done. That way, GLib
> > > would only be required for a few tests.
> > > 
> > > I don't know what maintainers thinks about it?
> > > 
> > 
> > This is a good idea to have as little as possible number of
> > external libs linked. I am afraid removing glib would be hard as
> > there are some other parts used in igt, like udev changes
> > monitoring.
> 
> I did not explore the whole codebase, just android/glib.h. I think that the
> tests requiring udev don't use glib but libudev?
> 
> > What about placing this code in lib with name igt_ini.c?
> > Or igt_config.c?
> 
> There is already igt_rc.h, why not igt_rc.c?
> 

Right, it is a good place. +cc Sebastian Brzezinka <[email protected]>

Regards,
Kamil

> > +cc Ashutosh, Karthik, Swati and Zbigniew
> > 
> > > > [How]
> > > > Expand the dummy GKeyFile structure to record the file path upon calling
> > > > g_key_file_load_from_file. Implement a minimal internal INI parser function
> > > > (__android_g_key_file_get) inside g_key_file_get_string to traverse the file
> > > > line-by-line, matching requested [group] and key strings, stripping trailing
> > > > newlines, and returning a heap-allocated duplicate string (strdup).
> > > > 
> > > > Signed-off-by: Mark Yacoub <[email protected]>
> > > 
> > > Anyway, if there is a v2 of this patch, can you run checkpatch and fix the
> > > issues?
> > > 
> > > > ---
> > > >    include/android/glib.h | 120 +++++++++++++++++++++++++++++++++++++----
> > > >    1 file changed, 111 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/include/android/glib.h b/include/android/glib.h
> > > > index 0b6658287..5b8ba86c7 100644
> > > > --- a/include/android/glib.h
> > > > +++ b/include/android/glib.h
> > > > @@ -16,8 +16,7 @@ typedef struct _GError {
> > > >    } GError;
> > > >    typedef struct _GKeyFile {
> > > > -	char *key;
> > > > -	char *value;
> > > > +	char path[512];
> > 
> > Why did you drop key/value from here?
> > 
> > > >    } GKeyFile;
> > > >    typedef struct _GMatchInfo {
> > [cut]
> > 
> > Regards,
> > Kamil
>