The following reply was made to PR kern/36673; it has been noted by GNATS.

From: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
To: juan@xtrarom.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/36673: dubious code in sysmon_envsys
Date: Sun, 22 Jul 2007 00:45:02 +0900 (JST)

 > > 	- sme_make_dictionary should fully set up a dictionary before
 > > 	 adding it to the array.  (instead of leaving a partial
 > > dictionary on errors.)
 > > 	 same for sysmon_envsys_createplist and its array.
 > 
 > Are you saying that only one error should destroy the whole array?
 > and why?
 
 because it's the simplest way to handle errors.
 and it's clearer, at least than leaving partially initialized data.
 
 > > 	- what global mutexes like sme_mtx protect is not clear.
 > 
 > Not clear why? can you explain more?
 
 because it isn't documented.  it might be clear to you, but not to me.
 
 > > 	- '--sme->sme_nsensors' in sme_update_dictionary seems quite
 > > dubious.
 > 
 > Dubious, why?
 
 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?
 
 > how do you want to skip sensors with duplicate
 > description, otherwise? let me know if you know any other way and I'll
 > fix.
 
 define "skip sensors" precisely.  otherwise i can't say how you can do it.
 
 i often feel you are assuming your intention is obvious to your peer.
 actually it isn't, especially when the code seems broken.
 
 > > 	- sysmon_envsys_createplist seems to ignore most errors
 > > 	 unless it was for the last sensor in sme_sensor_data[].
 > > 	 i don't understand why the last one is special.
 > 
 > I don't understand what you are reporting here. 
 
 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.
 
 YAMAMOTO Takashi