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
