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 02:33:34 +0900 (JST)

 > On Sat, 21 Jul 2007 16:30:02 +0000 (UTC)
 > yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi) wrote:
 > 
 > >  to me, the locking here seems far from obvious.  for example, to me,
 > >  
 > >  - it isn't clear which of sme_mtx or sme_list_mtx protects
 > > sysmon_envsys_list. 
 > 
 > This should be sme_list_mtx for the LIST. I fixed most cases, but
 > perhaps there are still some, I'll fix.
 
 then, sysmonioctl_envsys seems broken.
 
 > >  - it isn't clear if it's safe to drop sme_mtx in the middle of
 > >   sme_make_dictionary.
 >  
 > Can I allocate/deallocate memory with kmem(9) with a mutex held?
 > someone said that it's not possible because it could cause a deadlock
 > or something.
 
 it depends.
 
 what i want to know is if it's safe to drop the lock here,
 rather than a reason why you want to drop it...
 
 > >  although i'm not sure what you want to do here,
 > >  a usual way to "skip" an entry in a loop is just "continue".
 > >  
 > >  	for (i = 0; i < sme->sme_nsensors; i++) {
 > >  		edata = &sme->sme_sensor_data[i];
 > >  		if (edata->flags & ENVSYS_FDUPDESC) {
 > >  			continue;
 > >  		}
 > >  
 > >  can you explain why you want to modify sme_nsensors here?
 > >  it isn't obvious to me.
 > 
 > Sure. In sme_make_dictionary the sensors with a duplicate description
 > do not create another dictionary in the array, so the number of
 > dictionaries in the array doesn't match with sme_nsensors.
 > 
 > So let's see this code in sme_update_dictionary():
 > 
 >         for (i = 0; i < sme->sme_nsensors; i++) {
 >                 edata = &sme->sme_sensor_data[i];
 >                 if (edata->flags & ENVSYS_FDUPDESC) {
 >                         --sme->sme_nsensors;
 >                         continue;
 >                 }
 > 
 > With the code in sys/lkm/misc/envsys2, there are 15 sensors in total
 > and sensors with index 13 and 14 have a duplicate description.
 > 
 > If sme_nsensors is not decreased the following code:
 > 
 >                 /* retrieve sensor's dictionary. */
 >                 dict = prop_array_get(array, i);
 >                 if (prop_object_type(dict) != PROP_TYPE_DICTIONARY) {
 >                         DPRINTF(("%s: not a dictionary (%d:%s)\n",
 >                             __func__, edata->sensor, sme->sme_name));
 >                         return EINVAL;
 >                 }
 > 
 > will fail, because 'i' will still try to access to the index specified
 > in sme_nsensors, but the dictionary doesn't exist and it will fail.
 > 
 > Is it clear for you now?
 
 what happens when you call sme_update_dictionary again?
 you decrease sme_nsensors again erroneously, don't you?
 
 i guess it's clearer to always use same indexes for sme_sensor_data and
 prop_array.  ie. use prop_array_set instead of prop_array_add.
 
 YAMAMOTO Takashi