[lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[Andre Prendel]

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

Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[Jean Delvare]

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

Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[Andre Prendel]

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

Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[Jean Delvare]

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

Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[Andre Prendel]

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

Re: [lm-sensors] [PATCH v2 5/7] sensord: Refactoring of loadConfig()

[Jean Delvare]

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