Comment 35 for bug 1769236

Revision history for this message
Glen Ditchfield (gjditchfield) wrote :

I tried thermald 1.7.2, and I can make it work for me, but I don't
think it is the best solution. Please pardon the length at which I
explain my confusion.

I find the thermal-conf files to be confusing. Ubuntu 18.04's
thermald-1.7.0 package installs file /etc/thermald/thermal-conf.xml.
Based on the way other packages are configured, I would expect this to
be the configuration file, but thermald-1.7.0 ignores it. It seems to
be just a collection of examples.

Is that /etc/thermald/thermal-conf.xml file really suitable for every
system to use as a default configuration? If it is not OK, then the file
probably should be placed in /usr/share/doc instead of /etc.

The thermal-conf.xml.auto file that 1.7.0 generates for my system
seems to be OK, except for the Temperature element. Do you see any
other problems with it? As far as I know, my only problem is that I
can't override the generated Temperature.

My system runs well if I stop thermald. I don't know how much better
it would run *with* thermald, but "just stop thermald" seems to be an
acceptable fall-back position.

The new thermald-1.7.2 also attempts to read
/etc/thermald/thermal-conf.xml.auto, and I could edit a generated
thermal-conf.xml.auto to correct the Temperature element and put it
there. However, the ".auto" suffix would be misleading. I would
rather put the edited file in /etc/thermal-conf.xml.

Thermald-1.7.2 *only* reads the files in /etc if it *can't* generate
the /var/run file. It still doesn't provide a way to override a
successfully generated file. What if the generated file has a bug in
it? What if the user wants to fine-tune the configuration in some way?

Now consider what happens when I install thermald-1.7.2 on my 18.04
system. Similar things could happen to other people in the future, if
Cosmic Cuttlefish adopts 1.7.2 and other people upgrade to Cosmic from
18.04.

- Because of the int3400 check in line 169 of thd_trt_art_reader.cpp,
thermald does not try to generate
/var/run/thermald/thermal-conf.xml.auto.

- Then cthd_parse::parser_init() looks for
  /etc/thermald/thermal-conf.xml.auto, and does not find
  it.

- Finally cthd_parse::parser_init() looks for
  /etc/thermald/thermal-conf.xml, finds the
  "examples" file, and loads it. As I mentioned above, I don't know
  if that's a good idea.

---------

I'd like to suggest the following:

- The thermald package should *not* install
  /etc/thermald/thermal-conf.xml.

- thermald should first look for /etc/thermald/thermal-conf.xml and
  load it if it exists.

- If /etc/thermald/thermal-conf.xml does not exist, thermald should
  generate /var/run/thermald/thermal-conf.xml.auto and load it.

---------

Two unimportant points:

- The new thd_log_info message at thd_trt_art_reader.cpp, line 170,
should end with a \n.

- I recommend that cthd_parse::parser_init() log the chosen
xml_config_file's name every time, not just if it chooses the
generated file.