Subject: Re: kern/36673: dubious code in sysmon_envsys
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Juan RP <juan@xtrarom.org>
List: netbsd-bugs
Date: 07/21/2007 16:20:02
The following reply was made to PR kern/36673; it has been noted by GNATS.

From: Juan RP <juan@xtrarom.org>
To: gnats-bugs@NetBSD.org
Cc: yamt@mwd.biglobe.ne.jp
Subject: Re: kern/36673: dubious code in sysmon_envsys
Date: Sat, 21 Jul 2007 18:19:35 +0200

 On Sat, 21 Jul 2007 16:05:03 +0000 (UTC)
 Juan RP <juan@xtrarom.org> wrote:
 
 >  > because i don't understand why you want to modify sme_nsensors
 >  > here.
 >  > 
 >  > besides, the current code:
 >  > 
 >  >         for (i = 0; i < sme->sme_nsensors; i++) {
 >  >                 edata = &sme->sme_sensor_data[i];
 >  >                 if (edata->flags & ENVSYS_FDUPDESC) {
 >  >                         --sme->sme_nsensors;
 >  >                         --i;
 >  >                         continue;
 >  >                 }
 >  > 
 >  > "--i; continue;" means "i" will be the same value in the next
 >  > iteration of the loop.  so it will look at the same "edata", and
 >  > the ENVSYS_FDUPDESC condition will always to be true until the
 >  > loop exits. so it's effectively same as:
 >  > 
 >  >         for (i = 0; i < sme->sme_nsensors; i++) {
 >  >                 edata = &sme->sme_sensor_data[i];
 >  >                 if (edata->flags & ENVSYS_FDUPDESC) {
 >  > 			sme->sme_nsensors = i;
 >  > 			break;
 >  >                 }
 >  > 
 >  > is it what you intend?
 >  
 >  But that would mean that if you break and it's not the last sensor,
 >  next sensors will be skipped... right? this is not what we want to
 > make it show all valid sensors.
 
 I removed this bogus '--i' and all works as expected. I think this was
 required in a previous implementation that I had, but not needed since
 I added a SLIST to handle the sensor descriptions.
   
 >  > suppose that sme->sme_nsensors == 2.
 >  > if an error happens for sme->sme_sensor_data[0], it will not be
 >  > returned to the caller of sysmon_envsys_createplist.  (unless it
 >  > was EINVAL) otoh, if an error happens for sme->sme_sensor_data[1],
 >  > it will be returned to the caller of sysmon_envsys_createplist.
 >  
 >  You are right. I changed the code to check for all errors, let me
 > know if there are more problems.
 
 Thanks. I think there's only one point to fix your PR right now?
 
 -- 
 Juan Romero Pardines 	- The NetBSD Project
 http://plog.xtrarom.org 	- NetBSD/pkgsrc news in Spanish