On Tue, Jun 29, 2021 at 09:55:58AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 28, 2021 at 04:03:14PM -0700, Matthias Kaehlcke wrote:
>
> [...]
>
> >
> >
> > A few more previous lines of code for context:
> >
> > int count = QMP_NUM_COOLING_RESOURCES;
> >
> > qmp->cooling_devs = devm_kcalloc(qmp->dev, count,
> > sizeof(*qmp->cooling_devs),
> > GFP_KERNEL);
> >
> > I would suggest to initialize 'count' to 0 from the start and pass
> > QMP_NUM_COOLING_RESOURCES to devm_kcalloc() rather than 'count',
> > instead of resetting 'count' afterwards.
>
> Yeah, I thought about it but the actual bug in the code is not resetting
> the count value to 0. So fixing this way seems a better option.
I don't agree that it's the better option. IMO it's clearer to pass
the constant QMP_NUM_COOLING_RESOURCES directly to devm_kcalloc(),
rather than giving the impression that the number of allocated items
is variable. Repurposing variables can be confusing and led to this
bug. Also the resulting code doesn't need to re-initialize 'count'.
On Tue, Jun 29, 2021 at 07:17:20AM -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 29, 2021 at 09:55:58AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Jun 28, 2021 at 04:03:14PM -0700, Matthias Kaehlcke wrote:
> >
> > [...]
> >
> > >
> > >
> > > A few more previous lines of code for context:
> > >
> > > int count = QMP_NUM_COOLING_RESOURCES;
> > >
> > > qmp->cooling_devs = devm_kcalloc(qmp->dev, count,
> > > sizeof(*qmp->cooling_devs),
> > > GFP_KERNEL);
> > >
> > > I would suggest to initialize 'count' to 0 from the start and pass
> > > QMP_NUM_COOLING_RESOURCES to devm_kcalloc() rather than 'count',
> > > instead of resetting 'count' afterwards.
> >
> > Yeah, I thought about it but the actual bug in the code is not resetting
> > the count value to 0. So fixing this way seems a better option.
>
> I don't agree that it's the better option. IMO it's clearer to pass
> the constant QMP_NUM_COOLING_RESOURCES directly to devm_kcalloc(),
> rather than giving the impression that the number of allocated items
> is variable. Repurposing variables can be confusing and led to this
> bug. Also the resulting code doesn't need to re-initialize 'count'.
I don't dis-agree with you on this :) Let me send v2 incorporating the
comments.
Thanks,
Mani