* [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()
@ 2009-05-11 16:04 Andre Prendel
2009-05-12 19:43 ` Jean Delvare
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Andre Prendel @ 2009-05-11 16:04 UTC (permalink / raw)
To: lm-sensors
This patch does some refactoring of the loadConfig()
function.
* Simplifying the conditions makes code flow clearer and eliminates
long lines (> 80 chars).
* Removed useless stat() call.
* Return -1 in error case, instead of several positive values (never defined).
Changes in v2:
Blank line before the first #include.
Fix typo.
Bring back logging (reload configuration).
Consistent error messages.
Drop reading configuration from stdin.
Fix logging (if fopen() fails).
Cleanup sensors before reloading configuration.
Fix compile warning.
Don't print error value if reloadLib() fails.
---
lib.c | 74 +++++++++++++++++++++++++++++++-------------------------------
sensord.c | 4 +--
2 files changed, 40 insertions(+), 38 deletions(-)
Index: quilt-sensors/prog/sensord/lib.c
=================================--- quilt-sensors.orig/prog/sensord/lib.c 2009-04-26 22:11:03.000000000 +0200
+++ quilt-sensors/prog/sensord/lib.c 2009-04-26 22:13:09.000000000 +0200
@@ -21,6 +21,7 @@
* MA 02110-1301 USA.
*/
+#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -34,46 +35,47 @@
static int loadConfig(const char *cfgPath, int reload)
{
- struct stat stats;
- FILE *cfg = NULL;
- int ret = 0;
-
- if (cfgPath && !strcmp(cfgPath, "-")) {
- if (!reload) {
- if ((ret = sensors_init(stdin))) {
- sensorLog(LOG_ERR,
- "Error loading sensors configuration file <stdin>: %s",
- sensors_strerror(ret));
- ret = 12;
- }
- }
- } else if (cfgPath && stat(cfgPath, &stats) < 0) {
- sensorLog(LOG_ERR,
- "Error stating sensors configuration file: %s",
- cfgPath);
- ret = 10;
- } else {
- if (reload) {
+ int ret;
+ FILE *fp;
+
+ /* Load default configuration. */
+ if (!cfgPath) {
+ if (reload) {
sensorLog(LOG_INFO, "configuration reloading");
- sensors_cleanup();
+ sensors_cleanup();
}
- if (cfgPath && !(cfg = fopen(cfgPath, "r"))) {
- sensorLog(LOG_ERR,
- "Error opening sensors configuration file: %s",
- cfgPath);
- ret = 11;
- } else if ((ret = sensors_init(cfg))) {
- sensorLog(LOG_ERR,
- "Error loading sensors configuration file %s: %s",
- cfgPath ? cfgPath : "(default)",
- sensors_strerror(ret));
- ret = 11;
- }
- if (cfg)
- fclose(cfg);
+
+ ret = sensors_init(NULL);
+ if (ret) {
+ sensorLog(LOG_ERR, "Error loading default"
+ " configuration file: %s",
+ sensors_strerror(ret));
+ return -1;
+ }
+ return 0;
+ }
+
+ fp = fopen(cfgPath, "r");
+ if (!fp) {
+ sensorLog(LOG_ERR, "Error opening config file %s: %s",
+ strerror(errno));
+ return -1;
+ }
+
+ if (reload) {
+ sensorLog(LOG_INFO, "configuration reloading");
+ sensors_cleanup();
}
+ ret = sensors_init(fp);
+ if (ret) {
+ sensorLog(LOG_ERR, "Error loading sensors configuration file"
+ " %s: %s", cfgPath, sensors_strerror(ret));
+ fclose(fp);
+ return -1;
+ }
+ fclose(fp);
- return ret;
+ return 0;
}
int loadLib(const char *cfgPath)
Index: quilt-sensors/prog/sensord/sensord.c
=================================--- quilt-sensors.orig/prog/sensord/sensord.c 2009-04-26 22:13:33.000000000 +0200
+++ quilt-sensors/prog/sensord/sensord.c 2009-04-26 22:13:49.000000000 +0200
@@ -93,8 +93,8 @@
if (reload) {
ret = reloadLib(sensord_args.cfgFile);
if (ret)
- sensorLog(LOG_NOTICE,
- "config reload error (%d)", ret);
+ sensorLog(LOG_NOTICE, "configuration reload"
+ " error");
reload = 0;
}
if (sensord_args.scanTime && (scanValue <= 0)) {
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()
2009-05-11 16:04 [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig() Andre Prendel
@ 2009-05-12 19:43 ` Jean Delvare
2009-05-13 8:09 ` Andre Prendel
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-05-12 19:43 UTC (permalink / raw)
To: lm-sensors
On Mon, 11 May 2009 18:04:11 +0200, Andre Prendel wrote:
> This patch does some refactoring of the loadConfig()
> function.
>
> * Simplifying the conditions makes code flow clearer and eliminates
> long lines (> 80 chars).
> * Removed useless stat() call.
> * Return -1 in error case, instead of several positive values (never defined).
>
> Changes in v2:
>
> Blank line before the first #include.
> Fix typo.
> Bring back logging (reload configuration).
> Consistent error messages.
> Drop reading configuration from stdin.
> Fix logging (if fopen() fails).
> Cleanup sensors before reloading configuration.
> Fix compile warning.
> Don't print error value if reloadLib() fails.
Looks almost OK, just one minor issue:
> ---
>
> lib.c | 74 +++++++++++++++++++++++++++++++-------------------------------
> sensord.c | 4 +--
> 2 files changed, 40 insertions(+), 38 deletions(-)
>
> Index: quilt-sensors/prog/sensord/lib.c
> =================================> --- quilt-sensors.orig/prog/sensord/lib.c 2009-04-26 22:11:03.000000000 +0200
> +++ quilt-sensors/prog/sensord/lib.c 2009-04-26 22:13:09.000000000 +0200
> (...)
> + /* Load default configuration. */
> + if (!cfgPath) {
> + if (reload) {
> sensorLog(LOG_INFO, "configuration reloading");
> - sensors_cleanup();
> + sensors_cleanup();
You're adding leading white spaces!
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()
2009-05-11 16:04 [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig() Andre Prendel
2009-05-12 19:43 ` Jean Delvare
@ 2009-05-13 8:09 ` Andre Prendel
2009-05-13 8:21 ` Jean Delvare
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Andre Prendel @ 2009-05-13 8:09 UTC (permalink / raw)
To: lm-sensors
On Tue, May 12, 2009 at 09:43:20PM +0200, Jean Delvare wrote:
> On Mon, 11 May 2009 18:04:11 +0200, Andre Prendel wrote:
> > This patch does some refactoring of the loadConfig()
> > function.
> >
> > * Simplifying the conditions makes code flow clearer and eliminates
> > long lines (> 80 chars).
> > * Removed useless stat() call.
> > * Return -1 in error case, instead of several positive values (never defined).
> >
> > Changes in v2:
> >
> > Blank line before the first #include.
> > Fix typo.
> > Bring back logging (reload configuration).
> > Consistent error messages.
> > Drop reading configuration from stdin.
> > Fix logging (if fopen() fails).
> > Cleanup sensors before reloading configuration.
> > Fix compile warning.
> > Don't print error value if reloadLib() fails.
>
> Looks almost OK, just one minor issue:
>
> > ---
> >
> > lib.c | 74 +++++++++++++++++++++++++++++++-------------------------------
> > sensord.c | 4 +--
> > 2 files changed, 40 insertions(+), 38 deletions(-)
> >
> > Index: quilt-sensors/prog/sensord/lib.c
> > =================================> > --- quilt-sensors.orig/prog/sensord/lib.c 2009-04-26 22:11:03.000000000 +0200
> > +++ quilt-sensors/prog/sensord/lib.c 2009-04-26 22:13:09.000000000 +0200
> > (...)
> > + /* Load default configuration. */
> > + if (!cfgPath) {
> > + if (reload) {
> > sensorLog(LOG_INFO, "configuration reloading");
> > - sensors_cleanup();
> > + sensors_cleanup();
>
> You're adding leading white spaces!
Hi Jean,
I really cannot see any whitespaces. What do you mean?
Thanks,
Andre
>
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()
2009-05-11 16:04 [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig() Andre Prendel
2009-05-12 19:43 ` Jean Delvare
2009-05-13 8:09 ` Andre Prendel
@ 2009-05-13 8:21 ` Jean Delvare
2009-05-13 9:07 ` Andre Prendel
2009-05-13 9:12 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-05-13 8:21 UTC (permalink / raw)
To: lm-sensors
On Wed, 13 May 2009 10:09:43 +0200, Andre Prendel wrote:
> On Tue, May 12, 2009 at 09:43:20PM +0200, Jean Delvare wrote:
> > On Mon, 11 May 2009 18:04:11 +0200, Andre Prendel wrote:
> > > This patch does some refactoring of the loadConfig()
> > > function.
> > >
> > > * Simplifying the conditions makes code flow clearer and eliminates
> > > long lines (> 80 chars).
> > > * Removed useless stat() call.
> > > * Return -1 in error case, instead of several positive values (never defined).
> > >
> > > Changes in v2:
> > >
> > > Blank line before the first #include.
> > > Fix typo.
> > > Bring back logging (reload configuration).
> > > Consistent error messages.
> > > Drop reading configuration from stdin.
> > > Fix logging (if fopen() fails).
> > > Cleanup sensors before reloading configuration.
> > > Fix compile warning.
> > > Don't print error value if reloadLib() fails.
> >
> > Looks almost OK, just one minor issue:
> >
> > > ---
> > >
> > > lib.c | 74 +++++++++++++++++++++++++++++++-------------------------------
> > > sensord.c | 4 +--
> > > 2 files changed, 40 insertions(+), 38 deletions(-)
> > >
> > > Index: quilt-sensors/prog/sensord/lib.c
> > > =================================> > > --- quilt-sensors.orig/prog/sensord/lib.c 2009-04-26 22:11:03.000000000 +0200
> > > +++ quilt-sensors/prog/sensord/lib.c 2009-04-26 22:13:09.000000000 +0200
> > > (...)
> > > + /* Load default configuration. */
> > > + if (!cfgPath) {
> > > + if (reload) {
> > > sensorLog(LOG_INFO, "configuration reloading");
> > > - sensors_cleanup();
> > > + sensors_cleanup();
> >
> > You're adding leading white spaces!
>
> Hi Jean,
>
> I really cannot see any whitespaces. What do you mean?
I mean:
<minus><tab><tab><tab>sensors_cleanup();
<plus><space><space><tab><tab><tab>sensors_cleanup();
The two spaces shouldn't be there. As a matter of fact, the effective
content of this line doesn't change, so it shouldn't show up in the
diff... unless there are invisible whitespace changes.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()
2009-05-11 16:04 [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig() Andre Prendel
` (2 preceding siblings ...)
2009-05-13 8:21 ` Jean Delvare
@ 2009-05-13 9:07 ` Andre Prendel
2009-05-13 9:12 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Andre Prendel @ 2009-05-13 9:07 UTC (permalink / raw)
To: lm-sensors
On Wed, May 13, 2009 at 10:21:47AM +0200, Jean Delvare wrote:
> On Wed, 13 May 2009 10:09:43 +0200, Andre Prendel wrote:
> > On Tue, May 12, 2009 at 09:43:20PM +0200, Jean Delvare wrote:
> > > On Mon, 11 May 2009 18:04:11 +0200, Andre Prendel wrote:
> > > > This patch does some refactoring of the loadConfig()
> > > > function.
> > > >
> > > > * Simplifying the conditions makes code flow clearer and eliminates
> > > > long lines (> 80 chars).
> > > > * Removed useless stat() call.
> > > > * Return -1 in error case, instead of several positive values (never defined).
> > > >
> > > > Changes in v2:
> > > >
> > > > Blank line before the first #include.
> > > > Fix typo.
> > > > Bring back logging (reload configuration).
> > > > Consistent error messages.
> > > > Drop reading configuration from stdin.
> > > > Fix logging (if fopen() fails).
> > > > Cleanup sensors before reloading configuration.
> > > > Fix compile warning.
> > > > Don't print error value if reloadLib() fails.
> > >
> > > Looks almost OK, just one minor issue:
> > >
> > > > ---
> > > >
> > > > lib.c | 74 +++++++++++++++++++++++++++++++-------------------------------
> > > > sensord.c | 4 +--
> > > > 2 files changed, 40 insertions(+), 38 deletions(-)
> > > >
> > > > Index: quilt-sensors/prog/sensord/lib.c
> > > > =================================> > > > --- quilt-sensors.orig/prog/sensord/lib.c 2009-04-26 22:11:03.000000000 +0200
> > > > +++ quilt-sensors/prog/sensord/lib.c 2009-04-26 22:13:09.000000000 +0200
> > > > (...)
> > > > + /* Load default configuration. */
> > > > + if (!cfgPath) {
> > > > + if (reload) {
> > > > sensorLog(LOG_INFO, "configuration reloading");
> > > > - sensors_cleanup();
> > > > + sensors_cleanup();
> > >
> > > You're adding leading white spaces!
> >
> > Hi Jean,
> >
> > I really cannot see any whitespaces. What do you mean?
>
> I mean:
>
> <minus><tab><tab><tab>sensors_cleanup();
> <plus><space><space><tab><tab><tab>sensors_cleanup();
>
> The two spaces shouldn't be there. As a matter of fact, the effective
> content of this line doesn't change, so it shouldn't show up in the
> diff... unless there are invisible whitespace changes.
>
Ah, now I got it, thanks. I didn't see the two spaces (have trusted the
Emacs indentation). I'll fix that before committing.
BTW, how do you review patches? In your mailclient or in external
tool? Sometimes diffs are very difficult to read.
Thank,
Andre
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()
2009-05-11 16:04 [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig() Andre Prendel
` (3 preceding siblings ...)
2009-05-13 9:07 ` Andre Prendel
@ 2009-05-13 9:12 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2009-05-13 9:12 UTC (permalink / raw)
To: lm-sensors
On Wed, 13 May 2009 11:07:20 +0200, Andre Prendel wrote:
> On Wed, May 13, 2009 at 10:21:47AM +0200, Jean Delvare wrote:
> > I mean:
> >
> > <minus><tab><tab><tab>sensors_cleanup();
> > <plus><space><space><tab><tab><tab>sensors_cleanup();
> >
> > The two spaces shouldn't be there. As a matter of fact, the effective
> > content of this line doesn't change, so it shouldn't show up in the
> > diff... unless there are invisible whitespace changes.
>
> Ah, now I got it, thanks. I didn't see the two spaces (have trusted the
> Emacs indentation). I'll fix that before committing.
>
> BTW, how do you review patches? In your mailclient or in external
> tool? Sometimes diffs are very difficult to read.
Depends on how complex the patch is. For simple patches, in my mail
client. For more complex patches, I save them, and test them using
quilt. "quilt diff" adds color to the patches, this makes them easier
to read. If the patch changes a lot of code, I often open the complete
source file to double-check the new code looks good, and also build the
code and look for warnings.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-05-13 9:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-05-11 16:04 [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig() Andre Prendel
2009-05-12 19:43 ` Jean Delvare
2009-05-13 8:09 ` Andre Prendel
2009-05-13 8:21 ` Jean Delvare
2009-05-13 9:07 ` Andre Prendel
2009-05-13 9:12 ` Jean Delvare
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.