[lm-sensors] 3.1.2 sensord duplicate RRD name problem

[lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Sergey Kvachonok]

sensord fails to start with the following messages:

sensord: Creating round robin database
sensord: Error creating RRD file: /var/log/sensord.rrd: Duplicate DS name: temp1

sensors output is like this:

coretemp-isa-0000
Adapter: ISA adapter
Core 0:      +26.0 C  (high = +76.0 C, crit = +100.0 C)

coretemp-isa-0001
Adapter: ISA adapter
Core 1:      +27.0 C  (high = +76.0 C, crit = +100.0 C)

it8718-isa-0290
Adapter: ISA adapter
Vcore:       +1.01 V  (min =  +0.00 V, max =  +4.08 V)
V_DDR2:      +1.90 V  (min =  +0.00 V, max =  +4.08 V)
+3.3V:       +3.38 V  (min =  +0.00 V, max =  +4.08 V)
in3:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
in4:         +0.38 V  (min =  +0.00 V, max =  +2.10 V)
in7:         +3.15 V  (min =  +0.00 V, max =  +4.08 V)
Vbat:        +3.18 V
CPU fan:     148 RPM  (min =   10 RPM)
System fan:    0 RPM  (min =    0 RPM)
temp1:       -55.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
temp2:        -2.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
MB temp:     +17.0 C  (low  = +127.0 C, high = +60.0 C)  sensor = thermal diode
cpu0_vid:   +3.300 V

I believe the problem is that different sensors provide results with
the same name 'temp1'.
Are there any solutions for this problem?

Thanks in advance,
Sergey.

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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Jean Delvare]

Hi Sergei,

On Thu, 4 Mar 2010 14:37:05 +0200, Sergey Kvachonok wrote:
> sensord fails to start with the following messages:
> 
> sensord: Creating round robin database
> sensord: Error creating RRD file: /var/log/sensord.rrd: Duplicate DS name: temp1
> 
> sensors output is like this:
> 
> coretemp-isa-0000
> Adapter: ISA adapter
> Core 0:      +26.0 C  (high = +76.0 C, crit = +100.0 C)
> 
> coretemp-isa-0001
> Adapter: ISA adapter
> Core 1:      +27.0 C  (high = +76.0 C, crit = +100.0 C)
> 
> it8718-isa-0290
> Adapter: ISA adapter
> Vcore:       +1.01 V  (min =  +0.00 V, max =  +4.08 V)
> V_DDR2:      +1.90 V  (min =  +0.00 V, max =  +4.08 V)
> +3.3V:       +3.38 V  (min =  +0.00 V, max =  +4.08 V)
> in3:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
> in4:         +0.38 V  (min =  +0.00 V, max =  +2.10 V)
> in7:         +3.15 V  (min =  +0.00 V, max =  +4.08 V)
> Vbat:        +3.18 V
> CPU fan:     148 RPM  (min =   10 RPM)
> System fan:    0 RPM  (min =    0 RPM)
> temp1:       -55.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
> temp2:        -2.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
> MB temp:     +17.0 C  (low  = +127.0 C, high = +60.0 C)  sensor = thermal diode
> cpu0_vid:   +3.300 V
> 
> I believe the problem is that different sensors provide results with
> the same name 'temp1'.
> Are there any solutions for this problem?

Thanks for reporting. this appears to be a regression in 3.1.2, as I am
able to reproduce the bug with this version but not with 3.1.1 on the
same machine.

Andre, a bisection points to the following change of yours:
http://www.lm-sensors.org/changeset/5792/lm-sensors/trunk/prog/sensord
Any idea?

-- 
Jean Delvare

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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Andre Prendel]

On Thu, May 06, 2010 at 10:34:42AM +0200, Jean Delvare wrote:
> Hi Sergei,

Hi,
 
> On Thu, 4 Mar 2010 14:37:05 +0200, Sergey Kvachonok wrote:
> > sensord fails to start with the following messages:
> > 
> > sensord: Creating round robin database
> > sensord: Error creating RRD file: /var/log/sensord.rrd: Duplicate DS name: temp1
> > 
> > sensors output is like this:
> > 
> > coretemp-isa-0000
> > Adapter: ISA adapter
> > Core 0:      +26.0 C  (high = +76.0 C, crit = +100.0 C)
> > 
> > coretemp-isa-0001
> > Adapter: ISA adapter
> > Core 1:      +27.0 C  (high = +76.0 C, crit = +100.0 C)
> > 
> > it8718-isa-0290
> > Adapter: ISA adapter
> > Vcore:       +1.01 V  (min =  +0.00 V, max =  +4.08 V)
> > V_DDR2:      +1.90 V  (min =  +0.00 V, max =  +4.08 V)
> > +3.3V:       +3.38 V  (min =  +0.00 V, max =  +4.08 V)
> > in3:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
> > in4:         +0.38 V  (min =  +0.00 V, max =  +2.10 V)
> > in7:         +3.15 V  (min =  +0.00 V, max =  +4.08 V)
> > Vbat:        +3.18 V
> > CPU fan:     148 RPM  (min =   10 RPM)
> > System fan:    0 RPM  (min =    0 RPM)
> > temp1:       -55.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
> > temp2:        -2.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
> > MB temp:     +17.0 C  (low  = +127.0 C, high = +60.0 C)  sensor = thermal diode
> > cpu0_vid:   +3.300 V
> > 
> > I believe the problem is that different sensors provide results with
> > the same name 'temp1'.
> > Are there any solutions for this problem?
> 
> Thanks for reporting. this appears to be a regression in 3.1.2, as I am
> able to reproduce the bug with this version but not with 3.1.1 on the
> same machine.
> 
> Andre, a bisection points to the following change of yours:
> http://www.lm-sensors.org/changeset/5792/lm-sensors/trunk/prog/sensord
> Any idea?

I will take a look at this tonight.

Thanks,
Andre
 
> -- 
> Jean Delvare
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Jean Delvare]

Hi again Sergey and Andre,

On Thu, 6 May 2010 10:34:42 +0200, Jean Delvare wrote:
> Hi Sergei,
> 
> On Thu, 4 Mar 2010 14:37:05 +0200, Sergey Kvachonok wrote:
> > sensord fails to start with the following messages:
> > 
> > sensord: Creating round robin database
> > sensord: Error creating RRD file: /var/log/sensord.rrd: Duplicate DS name: temp1
> > 
> > sensors output is like this:
> > 
> > coretemp-isa-0000
> > Adapter: ISA adapter
> > Core 0:      +26.0 C  (high = +76.0 C, crit = +100.0 C)
> > 
> > coretemp-isa-0001
> > Adapter: ISA adapter
> > Core 1:      +27.0 C  (high = +76.0 C, crit = +100.0 C)
> > 
> > it8718-isa-0290
> > Adapter: ISA adapter
> > Vcore:       +1.01 V  (min =  +0.00 V, max =  +4.08 V)
> > V_DDR2:      +1.90 V  (min =  +0.00 V, max =  +4.08 V)
> > +3.3V:       +3.38 V  (min =  +0.00 V, max =  +4.08 V)
> > in3:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
> > in4:         +0.38 V  (min =  +0.00 V, max =  +2.10 V)
> > in7:         +3.15 V  (min =  +0.00 V, max =  +4.08 V)
> > Vbat:        +3.18 V
> > CPU fan:     148 RPM  (min =   10 RPM)
> > System fan:    0 RPM  (min =    0 RPM)
> > temp1:       -55.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
> > temp2:        -2.0 C  (low  = +127.0 C, high = +127.0 C)  sensor = thermistor
> > MB temp:     +17.0 C  (low  = +127.0 C, high = +60.0 C)  sensor = thermal diode
> > cpu0_vid:   +3.300 V
> > 
> > I believe the problem is that different sensors provide results with
> > the same name 'temp1'.
> > Are there any solutions for this problem?
> 
> Thanks for reporting. this appears to be a regression in 3.1.2, as I am
> able to reproduce the bug with this version but not with 3.1.1 on the
> same machine.
> 
> Andre, a bisection points to the following change of yours:
> http://www.lm-sensors.org/changeset/5792/lm-sensors/trunk/prog/sensord
> Any idea?

I think I see what's going on. Andre's patch assumes that the feature
number and the label slot number are always the sync, while feature
numbers are per-chip values and label slot numbers are global. It
doesn't make a difference when there's a single monitoring chip on the
system (or sensord is run on a single chip on purpose, as I always do)
but it breaks as soon as there is more than one chip handled by sensord.

As usual the use of global variables make the code much harder to
understand, which certainly explains why I didn't see the bug when
reviewing the patch.

I've created a ticket to track this bug:
http://www.lm-sensors.org/ticket/2377

Andre, I remember commenting on exactly this on the first version of
your patch:
http://lists.lm-sensors.org/pipermail/lm-sensors/2009-June/026172.html

Quoting myself:
"As far as I can see, 'i' and 'num' have the same value throughout the
loop, so you might as well get rid of one of them."

I think my comment was correct given the code, but instead of just
simplifying the code as we finally did, we should have asked ourselves
why such a simplification was suddenly possible with your patch if it
was not beforehand. In fact your first patch was already wrong, because
you reset num to 0 at the wrong place.

I have a patch ready, I'll give it some testing to make sure I got it
right, and then I'll post it here.

-- 
Jean Delvare

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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Jean Delvare]

On Thu, 6 May 2010 14:20:56 +0200, Jean Delvare wrote:
> I have a patch ready, I'll give it some testing to make sure I got it
> right, and then I'll post it here.

Andre, can you please comment on the patch below? Sergey, any chance
you could test it? It works for me on two different machines, but I may
not have covered all possible cases.

---
 prog/sensord/rrd.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

--- lm-sensors.orig/prog/sensord/rrd.c	2009-11-05 10:18:11.000000000 +0100
+++ lm-sensors/prog/sensord/rrd.c	2010-05-06 17:54:06.000000000 +0200
@@ -137,9 +137,11 @@ static void rrdCheckLabel(const char *ra
 	}
 }
 
+/* Returns the number of features processed, or -1 on error */
 static int _applyToFeatures(FeatureFN fn, void *data,
 			    const sensors_chip_name *chip,
-			    const ChipDescriptor *desc)
+			    const ChipDescriptor *desc,
+			    int labelOffset)
 {
 	int i;
 	const FeatureDescriptor *features = desc->features;
@@ -147,7 +149,7 @@ static int _applyToFeatures(FeatureFN fn
 	const char *rawLabel;
 	char *label;
 
-	for (i = 0; i < MAX_RRD_SENSORS && features[i].format; ++i) {
+	for (i = 0; labelOffset + i < MAX_RRD_SENSORS && features[i].format; ++i) {
 		feature = features + i;
 		rawLabel = feature->feature->name;
 
@@ -158,11 +160,11 @@ static int _applyToFeatures(FeatureFN fn
 			return -1;
 		}
 
-		rrdCheckLabel(rawLabel, i);
-		fn(data, rrdLabels[i], label, feature);
+		rrdCheckLabel(rawLabel, labelOffset + i);
+		fn(data, rrdLabels[labelOffset + i], label, feature);
 		free(label);
 	}
-	return 0;
+	return i;
 }
 
 static ChipDescriptor *lookup_known_chips(const sensors_chip_name *chip)
@@ -184,7 +186,7 @@ static ChipDescriptor *lookup_known_chip
 
 static int applyToFeatures(FeatureFN fn, void *data)
 {
-	int i, i_detected, ret;
+	int i, i_detected, ret, labelOffset = 0;
 	const sensors_chip_name *chip, *chip_arg;
 	ChipDescriptor *desc;
 
@@ -197,9 +199,10 @@ static int applyToFeatures(FeatureFN fn,
 			if (!desc)
 				continue;
 
-			ret = _applyToFeatures(fn, data, chip, desc);
-			if (ret)
+			ret = _applyToFeatures(fn, data, chip, desc, labelOffset);
+			if (ret < 0)
 				return ret;
+			labelOffset += ret;
 		}
 	}
 	return 0;



-- 
Jean Delvare

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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Andre Prendel]

On Thu, May 06, 2010 at 06:12:52PM +0200, Jean Delvare wrote:
> On Thu, 6 May 2010 14:20:56 +0200, Jean Delvare wrote:
> > I have a patch ready, I'll give it some testing to make sure I got it
> > right, and then I'll post it here.
> 
> Andre, can you please comment on the patch below? Sergey, any chance
> you could test it? It works for me on two different machines, but I may
> not have covered all possible cases.

Jean, the patch works for me with two sensor chips :)

So I think it's enough for the moment. I've just realised that the code is 
still difficult to read. I hope we will find some time for more
simplification/cleanups.
 
> ---
>  prog/sensord/rrd.c |   19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> --- lm-sensors.orig/prog/sensord/rrd.c	2009-11-05 10:18:11.000000000 +0100
> +++ lm-sensors/prog/sensord/rrd.c	2010-05-06 17:54:06.000000000 +0200
> @@ -137,9 +137,11 @@ static void rrdCheckLabel(const char *ra
>  	}
>  }
>  
> +/* Returns the number of features processed, or -1 on error */
>  static int _applyToFeatures(FeatureFN fn, void *data,
>  			    const sensors_chip_name *chip,
> -			    const ChipDescriptor *desc)
> +			    const ChipDescriptor *desc,
> +			    int labelOffset)
>  {
>  	int i;
>  	const FeatureDescriptor *features = desc->features;
> @@ -147,7 +149,7 @@ static int _applyToFeatures(FeatureFN fn
>  	const char *rawLabel;
>  	char *label;
>  
> -	for (i = 0; i < MAX_RRD_SENSORS && features[i].format; ++i) {
> +	for (i = 0; labelOffset + i < MAX_RRD_SENSORS && features[i].format; ++i) {
>  		feature = features + i;
>  		rawLabel = feature->feature->name;
>  
> @@ -158,11 +160,11 @@ static int _applyToFeatures(FeatureFN fn
>  			return -1;
>  		}
>  
> -		rrdCheckLabel(rawLabel, i);
> -		fn(data, rrdLabels[i], label, feature);
> +		rrdCheckLabel(rawLabel, labelOffset + i);
> +		fn(data, rrdLabels[labelOffset + i], label, feature);
>  		free(label);
>  	}
> -	return 0;
> +	return i;
>  }
>  
>  static ChipDescriptor *lookup_known_chips(const sensors_chip_name *chip)
> @@ -184,7 +186,7 @@ static ChipDescriptor *lookup_known_chip
>  
>  static int applyToFeatures(FeatureFN fn, void *data)
>  {
> -	int i, i_detected, ret;
> +	int i, i_detected, ret, labelOffset = 0;
>  	const sensors_chip_name *chip, *chip_arg;
>  	ChipDescriptor *desc;
>  
> @@ -197,9 +199,10 @@ static int applyToFeatures(FeatureFN fn,
>  			if (!desc)
>  				continue;
>  
> -			ret = _applyToFeatures(fn, data, chip, desc);
> -			if (ret)
> +			ret = _applyToFeatures(fn, data, chip, desc, labelOffset);
> +			if (ret < 0)
>  				return ret;
> +			labelOffset += ret;
>  		}
>  	}
>  	return 0;
> 
> 
> 
> -- 
> Jean Delvare

Regards,
Andre


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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Jean Delvare]

Hi Andre,

On Thu, 6 May 2010 21:42:30 +0200, Andre Prendel wrote:
> On Thu, May 06, 2010 at 06:12:52PM +0200, Jean Delvare wrote:
> > On Thu, 6 May 2010 14:20:56 +0200, Jean Delvare wrote:
> > > I have a patch ready, I'll give it some testing to make sure I got it
> > > right, and then I'll post it here.
> > 
> > Andre, can you please comment on the patch below? Sergey, any chance
> > you could test it? It works for me on two different machines, but I may
> > not have covered all possible cases.
> 
> Jean, the patch works for me with two sensor chips :)
> 
> So I think it's enough for the moment.

I've committed the patch, thank you. Hans, Aurelien, you probably want
to backport the fix to the Fedora and Debian packages if you are using
version 3.1.2 already:
  http://www.lm-sensors.org/changeset/5835

> I've just realised that the code is 
> still difficult to read. I hope we will find some time for more
> simplification/cleanups.

This would be very welcome. There has to be a way to come up with more
simple, less likely to break code.

-- 
Jean Delvare

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

Re: [lm-sensors] 3.1.2 sensord duplicate RRD name problem

[Sergey Kvachonok]

On 5/6/10, Jean Delvare <khali@linux-fr.org> wrote:
> On Thu, 6 May 2010 14:20:56 +0200, Jean Delvare wrote:
>> I have a patch ready, I'll give it some testing to make sure I got it
>> right, and then I'll post it here.
>
> Andre, can you please comment on the patch below? Sergey, any chance
> you could test it? It works for me on two different machines, but I may
> not have covered all possible cases.

smartd starts ok, RRD contains temp1 and temp1_0. Still no idea how to
tell which is which, but at least it starts and logs everything.

Thanks for your hard work,
Sergey

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