Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sawmill_logger: don't try to access sysfs entries when not existent #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

morphis
Copy link

@morphis morphis commented Nov 19, 2012

Without this we have the following in the system log:

2012-11-19T08:10:34.479387Z [45097] tuna user.err sleepd: sysfs:SysfsGetDouble: Failed to
open file '/sys/devices/w1 bus master/w1_master_slaves/getrawcoulomb': No such file or
directory
2012-11-19T08:10:34.479875Z [45097] tuna user.err sleepd: sysfs:SysfsGetDouble: Failed to
open file '/sys/devices/w1 bus master/w1_master_slaves/getcoulomb': No such file or
directory

If the mentioned sysfs node is not available we return -1 to indicate there is nothing we can report.

regards,
Simon

@kuta42
Copy link

kuta42 commented Nov 19, 2012

  • *rc = -1;
  • *c = -1;

Please write as -1.0 so that it's clear that the return value is a double.

If the mentioned sysfs node is not available we return -1 to indicate there is nothing we can report.

Does the caller recognize that -1.0 means "nothing we can report"?

@morphis
Copy link
Author

morphis commented Nov 19, 2012

Please write as -1.0 so that it's clear that the return value is double.

Done.

Does the caller recognize that -1.0 means "nothing we can report"?

The caller only prints the values to stdout and does nothing more with them. Reporting zero coulomb would let the user thing into the right direction when he see this in the log file.

@kuta42
Copy link

kuta42 commented Nov 20, 2012

Ah, I see now that the caller is generating a log entry to be processed by our internal "sawmill" utility. My guess is that it's more likely to be able to handle values of 0.0 than values of -1.0, so how about returning the former?

Also, it seems wasteful to do the directory tests every time get_battery_coulomb_reading() called. These /sys directories aren't dynamic; if they aren't there the first time the routine is called, they'll never be there. A local static char* initialized to NULL could be used to store the directory to pass as the first argument to SysfsGetDouble() and be set the first time the routine is called; (char *)1 could be used to represent the case when neither of the /sys directories exist and the return values are to be set to 0.0 .

@kdopen
Copy link
Contributor

kdopen commented Nov 20, 2012

Please don't use "special" pointer values. A pointer is either == NULL, or it's "valid". Just have a static bool or int to flag whether the directories exist. The same logic that would have set the pointer to (char *)1 can set the flag, and the intent is clearer.

@kuta42
Copy link

kuta42 commented Dec 3, 2012

Agreed.

Without this we have the following in the system log:

2012-11-19T08:10:34.479387Z [45097] tuna user.err sleepd: sysfs:SysfsGetDouble: Failed to
open file '/sys/devices/w1 bus master/w1_master_slaves/getrawcoulomb': No such file or
directory
2012-11-19T08:10:34.479875Z [45097] tuna user.err sleepd: sysfs:SysfsGetDouble: Failed to
open file '/sys/devices/w1 bus master/w1_master_slaves/getcoulomb': No such file or
directory

Open-webOS-DCO-1.0-Signed-off-by: Simon Busch <morphis@gravedo.de>
@morphis
Copy link
Author

morphis commented Jan 7, 2013

I updated the implementation according to the latest review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants