/sys/class/power_supply/BAT0/model_name can contain non-utf8

[nicklan]

On my system:

$ cat /sys/class/power_supply/BAT0/model_name
5�+1^^�^^�9(-:'&��

Or:

$ hexdump /sys/class/power_supply/BAT0/model_name
0000000 8035 312b 801e 801e 2839 3a2d 2627 8080
0000010 000a
0000011

This means trying to get battery info always returns an error due to https://github.com/svartalf/rust-battery/blob/master/battery/src/platform/linux/device.rs#L36

I'd suggest a change to:

let model = builder.model().unwrap_or(None);

I suspect it's a bug in the kernel that this file is basically filled with junk, but it would be nice if the crate didn't totally fail in this case.

[svartalf]

Hi, @nicklan! This is a great idea, I'm definitely up for this change.

Yet, ignoring all potential errors seems to be way too general, instead we should handle only UTF-8 conversion errors and return None in that case.

To do that we can rewrite this function to handle UTF-8 errors separately: https://github.com/svartalf/rust-battery/blob/575757ce89c1869f9ccca7f577a47d5a382d027a/battery/src/platform/linux/sysfs/fs.rs#L95

and then use it here: https://github.com/svartalf/rust-battery/blob/575757ce89c1869f9ccca7f577a47d5a382d027a/battery/src/platform/linux/sysfs/source.rs#L317-L319

Feel free to write a PR, otherwise I'll see if I can do smth about it later.

[nicklan]

Okay cool. I'm happy to write such a PR. I do wonder though, why only deal with UTF-8 errors? Does not being able to read the model for some other reason really mean not wanting to return ANY battery info?

Another suggestion: make the model field itself a Result<String> rather than an Option<String>. That way any errors can be surfaced to the user and they can decide if the model is necessary or not.

I realize that's an API breaking change, so I understand if you'd prefer not to do that.

[svartalf]

I do wonder though, why only deal with UTF-8 errors? Does not being able to read the model for some other reason really mean not wanting to return ANY battery info?

As you might have noticed, get_string function already handles some weird cases and I don't want to silently ignore other hidden quirks, mostly because they are heavily undocumented and it would be great to understand what we are dealing with in here, even if it means that for some users crate will not work correctly for some time.

I realize that's an API breaking change, so I understand if you'd prefer not to do that.

Yeah, I'd rather not to change it right now, but your idea is worth to think about :)