On 2022/04/07 13:31, Moritz Buhl wrote: > Any insights? > On Mon, Jan 10, 2022 at 03:18:47PM +0100, Moritz Buhl wrote: > > Hi tech@, > > > > The return value of add_child_sensors is returned in add_sdr_sensor, > > which is called by get_sdr. get_sdr mallocs psdr and only frees it > > if add_sdr_sensor returns 0. The assumption is that psdr is attached > > to a list in add_child_sensors otherwise. This is not the case if > > the malloc for psensor fails, or read_sensor() equals 0. In any > > of these error cases psdr is leaked.
I think you mean "This is not the case if the malloc for psensor fails, or read_sensor() _DOES NOT_ equal 0"? Anyway, the diff makes sense and is ok sthen@ While I'm thinking of ipmi(4), any comments on https://marc.info/?l=openbsd-tech&m=164460370813852&w=2? (Return values in the ipmi.c code are pretty messy, mixture of rc and rv variable names, and 1/-1 as well as errno values, and the existing handling in read_sensor() seems a bit silly, though not functionally bad). > > Index: sys/dev/ipmi.c > > =================================================================== > > RCS file: /mount/openbsd/cvs/src/sys/dev/ipmi.c,v > > retrieving revision 1.115 > > diff -u -p -r1.115 ipmi.c > > --- sys/dev/ipmi.c 23 Jan 2021 12:10:08 -0000 1.115 > > +++ sys/dev/ipmi.c 10 Jan 2022 13:49:19 -0000 > > @@ -1374,7 +1374,7 @@ add_child_sensors(struct ipmi_softc *sc, > > int sensor_num, int sensor_type, int ext_type, int sensor_base, > > int entity, const char *name) > > { > > - int typ, idx; > > + int typ, idx, rc = 0; > > struct ipmi_sensor *psensor; > > #ifdef IPMI_DEBUG > > struct sdrtype1 *s1 = (struct sdrtype1 *)psdr; > > @@ -1415,11 +1415,12 @@ add_child_sensors(struct ipmi_softc *sc, > > dbg_printf(5, " reading: %lld [%s]\n", > > psensor->i_sensor.value, > > psensor->i_sensor.desc); > > + rc = 1; > > } else > > free(psensor, M_DEVBUF, sizeof(*psensor)); > > } > > > > - return (1); > > + return (rc); > > } > > > > /* Handle IPMI Timer - reread sensor values */ > > >
