intel / thermal_daemon

Thermal daemon for IA
GNU General Public License v2.0
539 stars 117 forks source link

Migrate from dbus-glib to GDBus #426

Closed smallorange closed 7 months ago

smallorange commented 9 months ago

Hi,

This work migrated the DBus backend to GDBus. Since dbus-glib will be deprecated, the applications depending on it should be migrated to use GDBus as the DBus backend. Notably, the thermald package, shipped by RHEL for its customers, is currently the only package dependent on glib-dbus. This patch migrates the DBus backend to GDBus and it also resolves the dependency issue of the packages. Furthermore, it brings benefits to various Linux distributions affected by the deprecation of dbus-glib.

Fixes: https://github.com/intel/thermal_daemon/issues/348, https://github.com/intel/thermal_daemon/issues/416

smallorange commented 8 months ago

Hi,

Could folks review this patch?

Thank you :)

spandruvada commented 8 months ago

Thanks. I am in process of testing.

zhang-rui commented 8 months ago

Hi, @smallorange A dumb question, if I want to convert intel_lpmd at https://github.com/intel/intel-lpmd to GDBus, what should I do? it seems that only patch 3/5 is the critical part for this transition?

smallorange commented 8 months ago

Hi,

I'll deep dive into lpmd code and reply to this later :)

zhang-rui commented 8 months ago

Great, really appreciated! @smallorange

smallorange commented 8 months ago

Hi,

The structure of lpmd is pretty similar to thermal_daemon. For the original implementation, dbus-binding-tool generated the structure of the DBus and you just need to put all the helper functions (dbus_interface_l_pmfo_rc_eon()...etc.) to the right place and then the Dbus API works. That is good since the helpers are already done.

For GDBus, the first thing is to fix the GObject declaration to fit the current version of Glib. It looks like the GObject fixing commit. and then we can focus on the main commit.. Make sure g_bus_own_name() is properly set, for example: org.freedesktop.intel_lpmd, and the callbacks are properly set, (especially on on_bus_aquired).

Look into thd_dbus_on_bus_acquired(). on_bus_aquired callback will be invoked when g_bus_own_name() is successfully done. First, we start to introspect the Dbus XML introspection file (intel_lpmd_dbus_interface.xml). Then, we can start to register all the necessary callbacks, for example, method_call, get_preoperty, and set_property. If no error returns, it should start working.

Moreover, the Dbus introspection file should be set as a gresource. Look in to the commit. glib-compile-resources will compile the dbus introspection file into a C source code. thd_dbus_load_introspection() contains a example for looking up the data from gresource. Moreover, replacing autoconf with meson is a better solution to compile Glib programs since only a few meson commands can generate the code you want, such as greource.

That is the minimum requirement for getting GDbus running.

If you have question or need help, please let me know :)

zhang-rui commented 8 months ago

Hi, @smallorange , really appreciate your help on this. I will ramp up on this and propose some patches later, will reach you for more details if I run into some problems. :) Really appreciated!

spandruvada commented 7 months ago

I get this build error make[2]: No rule to make target 'thermald-resource.c', needed by 'thermald-thermald-resource.o'. Stop. make[2]: Leaving directory '/home/spandruv/development/tools/thermal_daemon' make[1]: [Makefile:1530: all-recursive] Error 1 make[1]: Leaving directory '/home/spandruv/development/tools/thermal_daemon'

spandruvada commented 7 months ago

This diff was required to build: diff --git a/Makefile.am b/Makefile.am index 2830bd0..a0c051a 100644 --- a/Makefile.am +++ b/Makefile.am @@ -41,7 +41,8 @@ thermald_LDADD = \ $(EVDEV_LIBS)

BUILT_SOURCES = \

-thd_resources.c: $(top_srcdir)/thermald.gresource.xml +thermald-resource.c: $(top_srcdir)/thermald-resource.gresource.xml $(AM_V_GEN) glib-compile-resources --generate-source thermald-resource.gresource.xml

CLEANFILES = $(BUILT_SOURCES)

smallorange commented 7 months ago

Hi,

Thank you for testing this patch.

Sorry for my mistake. I've updated the Makefile and it should work. Btw, I'll take a leave on Monday and I'll come back to work on Tuesday. :)

spandruvada commented 7 months ago

Fix the build issue and applied.

q66 commented 7 months ago

this PR is incomplete, right? because the project still does not build without dbus-glib, it references the old API in various places and the build system still forces it in

smallorange commented 7 months ago

this PR is incomplete, right? because the project still does not build without dbus-glib, it references the old API in various places and the build system still forces it in Hi,

I finished all necessary GDbus implementation of it but I have not yet tweaked the dependency (dbus-glib-1) of it. I need to find a way to switch between dbus-binding-tool and glib-compile-resources for dbus-glib and gdbus in Makefile.am. I wonder if the dbus-glib can be completely removed from thermald or it has to be kept?

q66 commented 7 months ago

it has to be removed completely for this to make sense (because distros will be dropping dbus-glib)

you should not need dbus-binding-tool in the end, just remove the header inclusion and tweak things around it; the other problem is that not all assumptions around dbus-glib are gone yet (e.g. there are still calls to API to set up integration of glib mainloop with dbus in other places here)

smallorange commented 7 months ago

it has to be removed completely for this to make sense (because distros will be dropping dbus-glib)

For this, I hope @spandruvada can answer this or @zhang-rui can help with this. Should the dbus-glib be removed completely or keep it to support the user who use dbus-glib?

you should not need dbus-binding-tool in the end, just remove the header inclusion and tweak things around it; the other problem is that not all assumptions around dbus-glib are gone yet (e.g. there are still calls to API to set up integration of glib mainloop with dbus in other places here)

Right. Some of the dbus-glib placed in thd_cdev_modem.cpp should be removed but it is a bit complicated for me. I need to find a way to test it since I don't have the appropriate hardware to test it. According to the commit message "Added support for modem throttling on SoFIA-3GR", it seems to support throttling SoFIA-3GR platform.