On Mon, Nov 07, 2022 at 06:21:47PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 08:40:27PM +0530, Deepak R Varma wrote:
> > On Mon, Nov 07, 2022 at 04:20:27PM +0300, Dan Carpenter wrote:
> > > On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> > > > Simplify code by using min_t helper macro for logical evaluation
> > > > and value assignment. Use the _t variant of min macro since the
> > > > variable types are not same.
> > > > This issue is identified by coccicheck using the minmax.cocci file.
> > > >
> > > > Signed-off-by: Deepak R Varma <drv@xxxxxxxxx>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > 1. Revise patch description. No functional change.
> > > >
> > > > drivers/staging/most/video/video.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> > > > index ffa97ef21ea5..d5cc7eea3b52 100644
> > > > --- a/drivers/staging/most/video/video.c
> > > > +++ b/drivers/staging/most/video/video.c
> > > > @@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
> > > > while (count > 0 && data_ready(mdev)) {
> > > > struct mbo *const mbo = get_top_mbo(mdev);
> > > > int const rem = mbo->processed_length - fh->offs;
> > > > - int const cnt = rem < count ? rem : count;
> > > > + int const cnt = min_t(int, rem, count);
> > >
> > > TL;DR use size_t instead of int.
> >
> > Hi Dan,
> > Thank you for reviewing the patch. Please see my queries inline.
> >
> > >
> > > Using "int" here is wrong. size_t is unsigned long meaning that it has
> > > 64 bits to use to represent positive values. (Let's ignore 32 bit
> > > arches). You have chopped it down to say that it now has 31 bits for
> > > positives and if BIT(31) is set then treat it as negative. Everything
> > > which is larger than INT_MAX will be broken.
> >
> > I did worry about the truncation int might cause to the size_t variable,
> > however, as the result is being assigned to an int, I decided to go for int to
> > be the typecast for min_t.
>
> Let's ignore that other layers prevent "count" from being greater than
> INT_MAX. mbo->processed_length is a u16. Also if "fh->offs" is more
> than mbo->processed_length that's a separate bug and we are already
> screwed.
Yes, "u16 - u32" looks wrong on the face, but it should always evaluat
positive, else we would have had other issues down the line.
>
> So that means rem is a relatively small number. A small number can
> easily fit in "int cnt". So we are eating a big pie ("count") but we
> are taking small bites ("cnt"). Everything works fine.
>
> But if we chop the pie in half or treat it as negative pie then the
> math breaks.
That makes perfect sense. Thank you so much Dan for the explanation.
>
> >
> > Also, won't size_t will force the int rem to be treated as unsigned value which
> > will impact the comparison when rem indeed is negative. If rem will never be
> > -ve, my worry will be void.
>
> Is "-ve" the TikTok way of abbreviating negative? Am I old?
No no... sorry. My fingers were catching up with the thoughts. :)
>
> The small bites are always positive. But if we are eating negative
> pie then we take negative size bites. min_t() should almost always use
> unsigned types. Everything else is a headache. I have often wondered
> why people do it but I think it's because of the 80 character rule and
> the word "int" is shorter than "unsigned long".
Understood. I will send in a revision with size_t type for min_t.
Thank you once again for the detailed answer. Much appreciate.
./drv
>
> regards,
> dan carpenter
>
>
