Re: [lm-sensors] [PATCH 8/8] sensord: Refactoring of rrdInit()

[Jean Delvare <khali@linux-fr.org>]

On Mon, 6 Apr 2009 09:58:10 +0200, Andre Prendel wrote:
> Refactoring of the rrdInit() function.
> 
> * Fix deep indentation levels
> * Return with unique error code (-1)
> ---
> 
> rrd.c |   85 ++++++++++++++++++++++++++++++------------------------------------
>  1 file changed, 39 insertions(+), 46 deletions(-)
> 
> --- quilt-sensors.orig/prog/sensord/rrd.c	2009-04-04 20:40:28.000000000 +0200
> +++ quilt-sensors/prog/sensord/rrd.c	2009-04-04 23:17:17.000000000 +0200
> @@ -242,58 +242,51 @@
>  
>  int rrdInit(void)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct stat tmp;
> +	char stepBuff[STEP_BUFF], rraBuff[RRA_BUFF];
> +	int argc = 4, num;
> +	const char *argv[6 + MAX_RRD_SENSORS] = {
> +		"sensord", sensord_args.rrdFile, "-s", stepBuff
> +	};
>  
>  	sensorLog(LOG_DEBUG, "sensor RRD init");
> -	if (stat(sensord_args.rrdFile, &tmp)) {
> -		if (errno = ENOENT) {
> -			char stepBuff[STEP_BUFF], rraBuff[RRA_BUFF];
> -			int argc = 4, num;
> -			const char *argv[6 + MAX_RRD_SENSORS] = {
> -				"sensord", sensord_args.rrdFile, "-s", stepBuff
> -			};
> -
> -			sensorLog(LOG_INFO, "creating round robin database");
> -			num = rrdGetSensors(argv + argc);
> -			if (num = 0) {
> -				sensorLog(LOG_ERR,
> -					  "Error creating RRD: %s: %s",
> -					  sensord_args.rrdFile,
> -					  "No sensors detected");
> -				ret = 2;
> -			} else if (num < 0) {
> -				ret = -num;
> -			} else {
> -				sprintf(stepBuff, "%d", sensord_args.rrdTime);
> -				sprintf(rraBuff, "RRA:%s:%f:%d:%d",
> -					sensord_args.rrdNoAverage ? "LAST" :
> -					"AVERAGE",
> -					0.5 /* fraction of non-unknown samples needed per entry */,
> -					1 /* samples per entry */,
> -					7 * 24 * 60 * 60 /
> -					sensord_args.rrdTime /* 1 week */);
> -
> -				argc += num;
> -				argv[argc ++] = rraBuff;
> -				argv[argc] = NULL;
> -				if ((ret = rrd_create(argc,
> -						      (char **) /* WEAK */ argv))) {
> -					sensorLog(LOG_ERR,
> -						  "Error creating RRD file: %s: %s",
> -						  sensord_args.rrdFile,
> -						  rrd_get_error());
> -				}
> -			}
> -		} else {
> -			sensorLog(LOG_ERR, "Error stat()ing RRD: %s: %s",
> -				  sensord_args.rrdFile, strerror(errno));
> -			ret = 1;
> +
> +	/* Create RRD if it does not exist. */
> +	ret = stat(sensord_args.rrdFile, &tmp);
> +	if (ret = -1 && errno != ENOENT) {
> +		sensorLog(LOG_ERR, "Could not stat rrd file: %s\n",
> +			  sensord_args.rrdFile);
> +		return -1;
> +	} else if (errno = ENOENT) {

"else" after a return statement isn't that useful ;)

But more importantly, ret may be != -1 at this point, in which case you
have no guarantee that errno is up-to-date. It could carry an old error
value. The original code did handle this properly... at the price of an
additional level of indentation. Doesn't look unfeasible though:

	if (stat(sensord_args.rrdFile, &tmp)) {
		if (errno != ENOENT) {
			sensorLog(LOG_ERR, "Error stat()ing RRD: %s: %s",
				  sensord_args.rrdFile, strerror(errno));
			return -1;		
		}

		sensorLog(LOG_INFO, "Creating round robin database");
		[etc.]
	}

> +		sensorLog(LOG_INFO, "Creating round robin database");
> +
> +		num = rrdGetSensors(argv + argc);
> +		if (num < 1) {
> +			sensorLog(LOG_ERR, "Error creating RRD: %s: %s",
> +				  sensord_args.rrdFile, "No sensors detected");
> +			return -1;
> +		}
> +
> +		sprintf(stepBuff, "%d", sensord_args.rrdTime);
> +		sprintf(rraBuff, "RRA:%s:%f:%d:%d",
> +			sensord_args.rrdNoAverage ? "LAST" :"AVERAGE",
> +			0.5, 1, 7 * 24 * 60 * 60 / sensord_args.rrdTime);
> +
> +		argc += num;
> +		argv[argc++] = rraBuff;
> +		argv[argc] = NULL;
> +
> +		ret = rrd_create(argc, (char**) argv);
> +		if (ret = -1) {
> +			sensorLog(LOG_ERR, "Error creating RRD file: %s: %s",
> +				  sensord_args.rrdFile, rrd_get_error());
> +			return -1;
>  		}
>  	}
> -	sensorLog(LOG_DEBUG, "sensor RRD inited");
>  
> -	return ret;
> +	sensorLog(LOG_DEBUG, "sensor RRD initialized");
> +	return 0;
>  }
>  
>  #define RRDCGI "/usr/bin/rrdcgi"

Other than that, your cleanup look reasonable.

-- 
Jean Delvare

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