[lm-sensors] [RFC 0/4] Add support for multiple configuration files

[lm-sensors] [RFC 0/4] Add support for multiple configuration files

[Jean Delvare]

Hi all,

Here is a proposal to enable support for multiple configuration files
to libsensors. The original idea was to extend the configuration file
syntax to allow file inclusion [1]. I have chosen a different approach
similar to what many other tools to, that is: a directory where users
can put configuration files. This seems more flexible to me.

[1] http://www.lm-sensors.org/ticket/2174

In practice, this means that users (or configuration tools) can add
libsensors configuration files to directory /etc/sensors.d, and they
are processed exactly the same way as /etc/sensors.conf is. One clear
potential use of this mechanism is a tool which would identify the
mainboard and drop a mainboard-specific configuration file
to /etc/sensors.d, either downloaded from the network or copied from
some /usr/share/libsensors directory.

This patch set is composed of 3 preliminary patches which reorganize
the libsensors initialization code with no feature changes, and a 4th
patch implementing the new feature.

One important thing to notice is that this change makes bus statements
configuration file specific. While it would be possible to let bus
statements run from one configuration file to the next, it could result
in collisions when dropping several files into /etc/sensors.d (for
example one for the mainboard and one for the graphics adapter.) OTOH,
this also enforces the fact that bus statements have no effect beyond
configuration files, which basically means it will be difficult, if not
impossible, to implement ticket #2238 [2]. While we have been living
without it so far, there have been some discussions lately about the
effects of module load order and this ticket was one possible partial
solution to this problem.

[2] http://www.lm-sensors.org/ticket/2238

Another issue which still needs to be solved is the reporting of parse
errors. At the moment, the parse errors are identified by only a line
number, based on the assumption that there is only one possible
configuration file so the user knows where to look. Now that libsensors
supports several configuration files, the error messages become more
difficult to read. I will look into this later. I fear that a clean
solution would require an API change though.

I would welcome comments, reviews and test reports on these patches.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC 0/4] Add support for multiple configuration

[Matt Roberds]

On Wed, 4 Feb 2009, Jean Delvare wrote:
> Another issue which still needs to be solved is the reporting of parse
> errors.

If you are going to have multiple config files, it might also be good to
report what files are being used, even when all of them parse OK.  This
would help the user detect a config file with permissions problems, or
that a config file didn't make it into the config directory for some
reason.  Maybe this only happens when -v is on, or similar.

> I fear that a clean solution would require an API change though.

The basic problem seems to be that you fopen the file and then pass a
FILE* down to the parser; I don't think you can work backwards to the
file name once you have a FILE*.  The API change would probably be to
pass the filename down as a string, either instead of the FILE* so the
parser can feed it to fopen, or along with the FILE* so the parser can
include the name in error messages.

Without an API change, you could do something like this.  This is based
on your patch 4/4.  fprintf(stderr) would probably get replaced by
sensors_error calls, and some of it might depend on a command-line
switch.

---
static int add_config_from_dir(const char *dir)
{
   	int i, count, res;
   	struct dirent **namelist;

   	count = scandir(dir, &namelist, config_file_filter, alphasort);
   	if (count < 0) {
   		if (errno = ENOMEM)
   			sensors_fatal_error(__func__, "Out of memory");
   		return 0;
   	}

   	for (res = 0, i = 0; !res && i < count; i++) {
   		int len;
   		char path[NAME_MAX];
   		FILE *input;

   		len = snprintf(path, NAME_MAX, "%s/%s", dir,
   			       namelist[i]->d_name);
   		if (len >= NAME_MAX)
   			sensors_fatal_error(__func__, "File name too long");

+		fprintf(stderr, "opening config file '%s': ", path);

   		input = fopen(path, "r");
   		if (input) {
+			fprintf(stderr, "ok; parsing: ");
   			res = parse_config(input);
+			if(res) {
   				/* parse problem */
+				fprintf(stderr, "error\n");
+			} else {
+				fprintf(stderr, "ok\n");
   			}
   			fclose(input);
+		}
+		else {
+		/* permissions problem, file disappeared, etc */
+			fprintf(stderr, "error\n");
+		}


   		free(namelist[i]);
   	}
   	free(namelist);

   	return res;
}
---

This still leaves it up to the user to connect the reported errors to a
file name which might not be on the same line.  In other words, the user
will see things like this:

---
opening config file 'ibm-5150.txt': ok; parsing: line 10: iin5 not valid
line 12: inn6 not valid
error
opening config file 'radeon-9500-asc.txt': ok; parsing: ok
---

and it might not be immediately obvious that "line 12" still refers to
ibm-5150.txt.  Also the naked "error" might be confusing.

In perfect happy world, something like this might be clearer:

---
opening ibm-5150.txt
ibm-5150.txt line 10: iin5 not valid
ibm-5150.txt line 12: inn6 not valid
opening radeon-9500-asc.txt
---

Matt Roberds


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC 0/4] Add support for multiple configuration

[Jean Delvare]

Hi Matt,

On Wed, 4 Feb 2009 20:29:40 -0600 (CST), Matt Roberds wrote:
> On Wed, 4 Feb 2009, Jean Delvare wrote:
> > Another issue which still needs to be solved is the reporting of parse
> > errors.
> 
> If you are going to have multiple config files, it might also be good to
> report what files are being used, even when all of them parse OK.  This
> would help the user detect a config file with permissions problems, or
> that a config file didn't make it into the config directory for some
> reason.  Maybe this only happens when -v is on, or similar.

We definitely don't want to print which configuration files are used by
default, the end user shouldn't have to care. Permission problems
should be reported regardless (my patch currently fails to do so,
thanks for pointing it out.) We could have option "-v" to print this
information, or maybe a dedicated mode (--test-config) that would
solely test that the configuration files are OK, without actually
reading sensor values. This latter option seems more useful as it could
be used by automatic configuration tools to validate their work.

That being said, all approaches would first require to extend the
libsensors API, as currently the choice of the default configuration
file(s) is left to the library and opaque to the user. We don't have a
channel to report it back.

This however is somewhat beyond the scope of my patch set. I think it
is important to get support for multiple configuration files quickly,
because other improvements depend on that. The improvement of error
reporting can wait.

> > I fear that a clean solution would require an API change though.
> 
> The basic problem seems to be that you fopen the file and then pass a
> FILE* down to the parser; I don't think you can work backwards to the
> file name once you have a FILE*.

Correct. This is however not the primary problem I had in mind. What
worries me more is the prototype of sensors_default_parse_error(),
because it is part of the API so we can't change it.

The fact that the user-space programs pass a FILE* rather than a file
name is what makes it possible to read the configuration file from
stdin (in sensors) or virtually from anywhere the user wants (sandbox,
network...) While this feature never stroke me as terribly useful, this
must have been the motivation for this specific part of the API (that
was before I joined the project.)

Note that this isn't necessarily a problem as far as API changes are
concerned. The need to let the user know in which configuration file a
parse error occurred only exists when the user did _not_ pass a FILE*
for the configuration. If he/she did, then libsensors will only process
that specific file (or whatever it actually is) so there is no need for
disambiguation.

> The API change would probably be to
> pass the filename down as a string, either instead of the FILE* so the
> parser can feed it to fopen, or along with the FILE* so the parser can
> include the name in error messages.

Yes, passing the file name as an extra parameter makes sense. My
initial idea was to simply remember the file name in a global variable,
but this wouldn't work for all errors.

> Without an API change, you could do something like this.  This is based
> on your patch 4/4.  fprintf(stderr) would probably get replaced by
> sensors_error calls, and some of it might depend on a command-line
> switch.

I don't really want to abuse sensors_error() for informative or debug
messages. But stderr isn't an option either. As I said above, the
problem is that we really lack the proper channel to return non-error
information down to the library user.

> ---
> static int add_config_from_dir(const char *dir)
> {
>    	int i, count, res;
>    	struct dirent **namelist;
> 
>    	count = scandir(dir, &namelist, config_file_filter, alphasort);
>    	if (count < 0) {
>    		if (errno = ENOMEM)
>    			sensors_fatal_error(__func__, "Out of memory");
>    		return 0;
>    	}
> 
>    	for (res = 0, i = 0; !res && i < count; i++) {
>    		int len;
>    		char path[NAME_MAX];
>    		FILE *input;
> 
>    		len = snprintf(path, NAME_MAX, "%s/%s", dir,
>    			       namelist[i]->d_name);
>    		if (len >= NAME_MAX)
>    			sensors_fatal_error(__func__, "File name too long");
> 
> +		fprintf(stderr, "opening config file '%s': ", path);
> 
>    		input = fopen(path, "r");
>    		if (input) {
> +			fprintf(stderr, "ok; parsing: ");
>    			res = parse_config(input);
> +			if(res) {
>    				/* parse problem */
> +				fprintf(stderr, "error\n");
> +			} else {
> +				fprintf(stderr, "ok\n");
>    			}
>    			fclose(input);
> +		}
> +		else {
> +		/* permissions problem, file disappeared, etc */
> +			fprintf(stderr, "error\n");
> +		}
> 
> 
>    		free(namelist[i]);
>    	}
>    	free(namelist);
> 
>    	return res;
> }
> ---
> 
> This still leaves it up to the user to connect the reported errors to a
> file name which might not be on the same line.  In other words, the user
> will see things like this:
> 
> ---
> opening config file 'ibm-5150.txt': ok; parsing: line 10: iin5 not valid
> line 12: inn6 not valid
> error
> opening config file 'radeon-9500-asc.txt': ok; parsing: ok
> ---
> 
> and it might not be immediately obvious that "line 12" still refers to
> ibm-5150.txt.  Also the naked "error" might be confusing.

The more critical problem is that errors might happen later. The line
number is stored in several data structures for this reason. The most
common case is "set" statements which fail on "sensors -s". If you
didn't store the configuration file name when you had a chance, you
simply don't have it available when you need it.

> In perfect happy world, something like this might be clearer:
> 
> ---
> opening ibm-5150.txt
> ibm-5150.txt line 10: iin5 not valid
> ibm-5150.txt line 12: inn6 not valid
> opening radeon-9500-asc.txt
> ---

This would indeed be ideal, however this seems impossible to do without
an API change. One possibility would be to add a new callback with the
extra parameter, but there are still compatibility concerns. I'll think
about it.

Thanks for your comments.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC 0/4] Add support for multiple configuration

[Matt Roberds]

On Thu, 5 Feb 2009, Jean Delvare wrote:
> On Wed, 4 Feb 2009 20:29:40 -0600 (CST), Matt Roberds wrote:

> We could have option "-v" to print this information, or maybe a
> dedicated mode (--test-config) that would solely test that the
> configuration files are OK, without actually reading sensor values.

There might even be two levels to this option.  The first would just
test the config files and list their names; this is for user confidence
and automated testing.  The second might print details of the decisions
sensors is making as it parses through the various config files,
something like "make -d" does.

Whatever the details, I think there needs to be *some* way to let the
user know which config files are being examined.  (WARNING, potential
religious debate: Have you ever tried to configure qmail?  :)  )

> This however is somewhat beyond the scope of my patch set. I think it
> is important to get support for multiple configuration files quickly,
> because other improvements depend on that. The improvement of error
> reporting can wait.

I agree.  It might be helpful to add a couple of lines to the
documentation about what to do if problems with multiple config files
are suspected (like tell the user to chmod 644 /etc/sensors.d/*.txt or
similar), and that more debugging information about multiple config
files is being worked on.

>> The basic problem seems to be that you fopen the file and then pass a
>> FILE* down to the parser; I don't think you can work backwards to the
>> file name once you have a FILE*.
>
> Correct. This is however not the primary problem I had in mind. What
> worries me more is the prototype of sensors_default_parse_error(),
> because it is part of the API so we can't change it.

Just call the new function sensors4_default_parse_error() and slowly
deprecate the old one.  Should only take about 10 years.  :)

> The fact that the user-space programs pass a FILE* rather than a file
> name is what makes it possible to read the configuration file from
> stdin (in sensors) or virtually from anywhere the user wants (sandbox,
> network...)

It makes "sensors -c /dev/null" work, which is sometimes useful if
you've completely confused yourself, or are trying to get debug reports
from users.  Maybe in lieu of this, there should be a debug command-line
option to ignore all the config files.

> The need to let the user know in which configuration file a parse
> error occurred only exists when the user did _not_ pass a FILE* for
> the configuration. If he/she did, then libsensors will only process
> that specific file (or whatever it actually is) so there is no need
> for disambiguation.

I can think of a case where it might be useful to both pass in a FILE*
and have libsensors read from existing files.  Say somebody has two
files in the libsensors config directory, one for the motherboard and
one for a graphics card with a temperature sensor on it.  Then they get
a new graphics card.  They can move or chmod out the config file for
their old graphics card, but maybe they want libsensors to still read
the config file for their motherboard while they experiment with passing
in the correct configuration for their new graphics card via a FILE*.
(Of course, they can get the same effect by just putting trial versions
of their new graphics card config file in the libsensors directory.)
I don't know if this is common or something that should be supported,
but it might be a possibility.

> I don't really want to abuse sensors_error() for informative or debug
> messages. But stderr isn't an option either. As I said above, the
> problem is that we really lack the proper channel to return non-error
> information down to the library user.

Maybe libsensors needs to develop something like errno/perror() from the
C library.  You don't want to *waste* memory, but in these latter days
you don't have to limit yourself to a single int, either.

Or maybe it needs to make some kind of private log file (that is, not
sent to syslog) that interested user programs can parse.  File locking
and race conditions are annoying but at least it would get the
information out there _somewhere_.

Odd thought: Some C preprocessors support things like

#pragma emit "This is the debug version of foo.h"

which causes the message to print at preprocess time; would something
like that be of any use for the libsensors config files?  OTOH, printing
stuff is usually easier for humans to understand, not code that uses the
library.  (I have to remember that a big use case of lmsensors is via
user code linked to the library.  I don't use it that way myself - I
just look at the output of 'sensors' - but many people do use it that
way.)

> The more critical problem is that errors might happen later.

Ah, this is a point I hadn't picked up on.  I agree that this makes the
problem a little trickier.

Matt Roberds


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFC 0/4] Add support for multiple configuration

[Jean Delvare]

Hi Matt,

On Thu, 5 Feb 2009 20:10:35 -0600 (CST), Matt Roberds wrote:
> On Thu, 5 Feb 2009, Jean Delvare wrote:
> > On Wed, 4 Feb 2009 20:29:40 -0600 (CST), Matt Roberds wrote:
> 
> > We could have option "-v" to print this information, or maybe a
> > dedicated mode (--test-config) that would solely test that the
> > configuration files are OK, without actually reading sensor values.
> 
> There might even be two levels to this option.  The first would just
> test the config files and list their names; this is for user confidence
> and automated testing.  The second might print details of the decisions
> sensors is making as it parses through the various config files,
> something like "make -d" does.
> 
> Whatever the details, I think there needs to be *some* way to let the
> user know which config files are being examined.  (WARNING, potential
> religious debate: Have you ever tried to configure qmail?  :)  )

I see your point, and I agree it can be useful for debugging (both the
configuration files and the library itself). Whether we need this level
of power for something as simple as libsensors is an open question.
Whether we can implement this without API changes is another. Whether
I have any work time to invest in developing this feature is yet
another. If someone (you?) want to start working on this...

> > This however is somewhat beyond the scope of my patch set. I think it
> > is important to get support for multiple configuration files quickly,
> > because other improvements depend on that. The improvement of error
> > reporting can wait.
> 
> I agree.  It might be helpful to add a couple of lines to the
> documentation about what to do if problems with multiple config files
> are suspected (like tell the user to chmod 644 /etc/sensors.d/*.txt or
> similar),

I am a notorious hater of vague error messages which attempt to give
you general hints. Short but accurate error messages are much, much
better.

If the configuration files can't be read, we should return an error
code which says just that (SENSORS_ERR_ACCESS_R). Not that I expect
this to be a frequent problem: I can't remember a single case in the 4
or 5 years I've spent on the lm-sensors project.

> and that more debugging information about multiple config
> files is being worked on.

This is the kind of documentation which doesn't really help the user
and tend to get outdated quickly. For things that are being worked on,
the user should ask our ticket system.

> >> The basic problem seems to be that you fopen the file and then pass a
> >> FILE* down to the parser; I don't think you can work backwards to the
> >> file name once you have a FILE*.
> >
> > Correct. This is however not the primary problem I had in mind. What
> > worries me more is the prototype of sensors_default_parse_error(),
> > because it is part of the API so we can't change it.
> 
> Just call the new function sensors4_default_parse_error() and slowly
> deprecate the old one.  Should only take about 10 years.  :)

I'd rather avoid "sensors4" in the name. But something like
sensors_parse_error_wfn() (for "with file name") would make sense.
However the main problem IMHO is not to come up with a new function.
The problem is that we don't want to call both the original function
(sensors_parse_error) and the new one (sensors_parse_error_wfn or
whatever) on parse error. We should call just one. So there needs to be
some logic to decide which one. As I understand it, user-defined
functions should always be preferred, and the new function (with file
name) should be preferred over the old one (no file name). This should
be doable but we'll have to pay attention and make sure we don't
accidentally break any application.

> > The fact that the user-space programs pass a FILE* rather than a file
> > name is what makes it possible to read the configuration file from
> > stdin (in sensors) or virtually from anywhere the user wants (sandbox,
> > network...)
> 
> It makes "sensors -c /dev/null" work, which is sometimes useful if
> you've completely confused yourself, or are trying to get debug reports
> from users.  Maybe in lieu of this, there should be a debug command-line
> option to ignore all the config files.

Option "-c" is useful way beyond this. It lets the user test custom
configuration files before making them the system default. It can also
be used to have per-application configuration files.

One sad limitation of this mechanism is that you can only pass one
custom configuration file. This is a libsensors API limitation
(sensors_init takes only one file). Could be worked around with an
additional library function sensors_add_config(). This new function
could even take a file name rather than a FILE* as a parameter, to
solve the error reporting problem we have been discussing before.

That being said, I am not sure how needed this feature is in practice.
It's a little hard to tell right now, with support for multiple
configuration files at the library level being just in its early stage.

> > The need to let the user know in which configuration file a parse
> > error occurred only exists when the user did _not_ pass a FILE* for
> > the configuration. If he/she did, then libsensors will only process
> > that specific file (or whatever it actually is) so there is no need
> > for disambiguation.
> 
> I can think of a case where it might be useful to both pass in a FILE*
> and have libsensors read from existing files.  Say somebody has two
> files in the libsensors config directory, one for the motherboard and
> one for a graphics card with a temperature sensor on it.  Then they get
> a new graphics card.  They can move or chmod out the config file for
> their old graphics card, but maybe they want libsensors to still read
> the config file for their motherboard while they experiment with passing
> in the correct configuration for their new graphics card via a FILE*.
> (Of course, they can get the same effect by just putting trial versions
> of their new graphics card config file in the libsensors directory.)
> I don't know if this is common or something that should be supported,
> but it might be a possibility.

This example doesn't sound too relevant to me. During the test phase
you can certainly live without the rest of the configuration.

> > I don't really want to abuse sensors_error() for informative or debug
> > messages. But stderr isn't an option either. As I said above, the
> > problem is that we really lack the proper channel to return non-error
> > information down to the library user.
> 
> Maybe libsensors needs to develop something like errno/perror() from the
> C library.  You don't want to *waste* memory, but in these latter days
> you don't have to limit yourself to a single int, either.

We already have sensors_strerror() for this purpose. As I wrote above,
the problem isn't to report errors, this works fine already. The
problem is how to report informative messages.

> Or maybe it needs to make some kind of private log file (that is, not
> sent to syslog) that interested user programs can parse.  File locking
> and race conditions are annoying but at least it would get the
> information out there _somewhere_.

Libraries writing logs directly don't look terribly right. I believe
it's the application job to do that. What's missing is the channel
between the library and the application to carry the information and
debug messages.

Then again I'm fairly we can live without that. As a matter of fact we
did live without that for 10 years, and I can't remember anyone asking
for it. Now I'm not saying it can't be useful. I'm saying it's probably
something to write down and keep in mind for later, when we write the
next big, incompatible version of libsensors.

> Odd thought: Some C preprocessors support things like
> 
> #pragma emit "This is the debug version of foo.h"
> 
> which causes the message to print at preprocess time; would something
> like that be of any use for the libsensors config files?  OTOH, printing
> stuff is usually easier for humans to understand, not code that uses the
> library.  (I have to remember that a big use case of lmsensors is via
> user code linked to the library.  I don't use it that way myself - I
> just look at the output of 'sensors' - but many people do use it that
> way.)

I don't really want to touch the configuration file scanner and parser
if I can avoid it. That's really not an area I am familiar with.

> > The more critical problem is that errors might happen later.
> 
> Ah, this is a point I hadn't picked up on.  I agree that this makes the
> problem a little trickier.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors