nvme-print: print always celsius temperature value

[ikegami-t]

The fahrenheit value also printed if used by the locale country.

[ikegami-t]

Fixes for the issue #2910 and #2456 as below for example.

1. locale country: US (used fahrenheit)

• stdout ouput

root@tokunori-X570-Taichi:/home/tokunori/nvme-cli# .build/nvme smart-log /dev/nvme0
...
temperature                             : 41 °C (314 K, 105 °F)
...
Temperature Sensor 1                    : 41 °C (314 K, 105 °F)
Temperature Sensor 2                    : 50 °C (323 K, 122 °F)
...

• json output

root@tokunori-X570-Taichi:/home/tokunori/nvme-cli# .build/nvme get-feature /dev/nvme1 -o json | grep TM
      "Temperature Threshold Hysteresis (TMPTHH)":"-273 Celsius",
      "TMPTHH kelvin":"0 K",
      "TMPTHH fahrenheit":"-459 Fahrenheit",
      "Threshold Temperature Select (TMPSEL)":0,
      "TMPSEL description":"Composite Temperature",
      "Temperature Threshold (TMPTH)":"77 Celsius",
      "TMPTH kelvin":"350 K",
      "TMPTH fahrenheit":"170 Fahrenheit"
      "Thermal Management Temperature 1 (TMT1)":"344 K",
      "TMT1 celsius":"71 Celsius",
      "TMT1 fahrenheit":"159 Fahrenheit",
      "Thermal Management Temperature 2 (TMT2)":"348 K",
      "TMT2 celsius":"75 Celsius",
      "TMT2 fahrenheit":"167 Fahrenheit"

2. locale country: JP (not used fahrenheit)

• stdout ouput

tokunori@tokunori-X570-Taichi:~/nvme-cli$ nvme-build smart-log /dev/nvme0
...
temperature                             : 41 °C (314 K)
...
Temperature Sensor 1                    : 41 °C (314 K)
Temperature Sensor 2                    : 50 °C (323 K)
...

• json output

tokunori@tokunori-X570-Taichi:~/nvme-cli$ nvme-build get-feature /dev/nvme1n1 -o json | grep TM
      "Temperature Threshold Hysteresis (TMPTHH)":"-273 Celsius",
      "TMPTHH kelvin":"0 K",
      "Threshold Temperature Select (TMPSEL)":0,
      "TMPSEL description":"Composite Temperature",
      "Temperature Threshold (TMPTH)":"77 Celsius",
      "TMPTH kelvin":"350 K"
      "Thermal Management Temperature 1 (TMT1)":"344 K",
      "TMT1 celsius":"71 Celsius",
      "Thermal Management Temperature 2 (TMT2)":"348 K",
      "TMT2 celsius":"75 Celsius"

[martin-belanger]

No need to copy to json_str. You can simply do:

obj_add_str(r, k, fahrenheit);

[ikegami-t]

Just fixed as mentioned. Thank you!

[wdoekes]

Thanks for picking this up 👍 Note that you're not yet fixing the setlocale (ab)use I mention in #2910

edit: Or one could just lose the check and print all three always. Like suggested in #2456 (comment) .

[ikegami-t]

Thanks for your review comment. Yes still using the locale information but it is only for the fahrenheit value output as optional so usually the celsius and kelvin values only output. Is it still really a problem to use the locale information?

temperature                             : 41 °C (314 K)

Note: If the locale country is US or some other countries the fahrenheit value is output as below (as probably as you understood the changes already). By the way about the locale countries before we considered it by the PR comment: #2381 (comment) so seems for me it is not abused the locale information as considered correctly before.

temperature                             : 41 °C (314 K, 105 °F)

[wdoekes]

Is it still really a problem to use the locale information?

I don't mind that you're showing extra details. But then I wonder why make it conditional based on the locale. Might as well show all three always.

so seems for me it is not abused the locale information as considered correctly before.

The information is correct. No doubt. You checked wikipedia and all that.

But the LC_MEASUREMENT setting is not used as it should be.

• If/when the Americans decide to be sensible and move to metric, the locale database (/usr/share/i18n/locales/en_US) would be updated by the OS and that change would be reflected in nvme-cli without you having to hand edit the hardcoded list.
• If someone uses a custom hand-hacked locale on their system, they would also get their preferred setting, and not your hardcoded list.

This is not a hill I'm willing to die on. Just know that you could use the correct setting (and it's fewer lines of code).

[wdoekes]

NOTE: In #2456 (comment) @ell1e informs us that nvme-cli fails to compile with musl since using LC_MEASUREMENT. That could be another argument to drop the conditionality and just always print all three units.

[ikegami-t]

The fahrenheit value usually not necessary so seems better to be not print basically. (For me only celsius value needed but NVMe specification used kelvin value. But I thought for US and some other counteries better to print the fahrenheit value.)
Could you please let me know about the correct LC_MEASUREMENT usecase? (Still looks the current nvme-cli LC_MEASUREMENT usecase not invalid for me.)
About the build error fixed by the commit: 9444ce0. (Sorry for the error.)
Note: Anyway really required all three values to print I will fix the commit again as so. (But sorry let me understand the issue more.)

[wdoekes]

About the build error fixed by the commit: 9444ce0. (Sorry for the error.)

Oh. In that case, all good. Carry on!

---

Anyway really required all three values to print I will fix the commit again as so.

I don't care about the conditionality now, as long as Celsius stays where it is :)

Still looks the current nvme-cli LC_MEASUREMENT usecase not invalid for me.

It "works", but its usage is wrong. You're violating how the locale functions should work:

• by hardcoding language properties ("US", ...)
• instead of using the framework as it exists (_NL_MEASUREMENT_MEASUREMENT)

I tried to explain how it "should be properly implemented" in #2456 (comment)

On the other hand, with the 9444ce0 fix, your version is now more compatible between glibc/musl than any fix using _NL_MEASUREMENT_MEASUREMENT would be, so there's that.

Like I said. Not a hill I'm willing to die on.

[ikegami-t]

Thank you so much for your explanation. I could understand much then I tried to fix the commit to use nl_langinfo() as suggested but unfortunally nvme-cli sets LC_ALL=C for other purpose then always the fahrenheit value not printed by using the function..

int argconfig_parse(int argc, char *argv[], const char *program_desc,
		    struct argconfig_commandline_options *options)
{
...
	if (!argconfig_check_human_readable(options))
		setlocale(LC_ALL, "C");

(Added)
The LC_ALL=C issue resolved then just pushed the patch to use nl_langinfo() instead.

[wdoekes]

b35405b LGTM 👍

But, you'll likely need some conditional compilation for musl, e.g.:

#ifndef LC_MEASUREMENT
static bool is_temperature_fahrenheit(void) { return false; }
#endif

[ikegami-t]

Thanks for your review comment! (Sorry for the error.) Just fixed the patch to check the LC_MEASUREMENT to build.

[igaw]

So after digging through this, I suggest that we print "Kelvin (Celsius Fahrenheit)" independent of the locale configuration. The standard is only using Kelvin as unit add the values in the parenthesis are for convenience. But this is just for the stdout case.

For the JSON output we shouldn't change it at all. This will break any existing parser. Maybe something for the next major version but I am really not happy to touch this part at all. It leads just to regressions.

Furthermore, the JSON output should likely not trying to be smart and only print the Kelvin values anyway. The locale thing is as the name says local.

[ell1e]

If stdout is what the average user sees after typing nvme smart-log, I highly recommend printing Celsius and potentially Fahrenheit too. I feel like the listing should tell the average user whether there is currently a problem or not. For example, most users in Europe will intuitively get that if they see an 80 degree celsius listing that something is wrong. Even if Kelvin satisfies some standard on paper, the listing probably should use a practical approach.

[ikegami-t]

Fixed as mentioned then always printed all the celsius and kelvin, fahrenheit values also so no locale information checked. And removed the json changes also. Thank you.

[igaw]

Thanks!