linux-kernel - Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220330151658.GV12805@kadam>
Date:   Wed, 30 Mar 2022 18:16:58 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel@...labora.com, linux-media@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 19/24] media: rkvdec-h264: Add field decoding support

Yeah.  I'm aboslutely fine with whatever you do.  Some of the questions
you're asking occurred to me too but I don't have the answers.

> > > > > +		for (i = 0; i < builder->num_valid; i++) {
> > > > > +			struct v4l2_h264_reference *ref;
> > > > > +			u8 dpb_valid;
> > > > > +			u8 bottom;
> > > > 
> > > > These would be better as type bool.
> > > 
> > > I never used a bool for bit operations before, but I guess that can work, thanks
> > > for the suggestion. As this deviates from the original code, I suppose I should
> > > make this a separate patch ?
> > 
> > I just saw the name and wondered why it was a u8.  bool does make more
> > sense and works fine for the bitwise stuff.  But I don't really care at
> > all.
> 
> I'll do that in v2, in same patch, looks minor enough. I think if using bool
> could guaranty that only 1 or 0 is  possible, it would be even better, but don't
> think C works like this.

I'm not sure I understand.  If you assign "bool x = <any non-zero>;"
then x is set to true.  Do you want a static checker warning for if
<any non-zero> can be something other than one or zero?  The problem is
that people sometimes deliberately do stuff like "bool x = var & 0xf0;".
Smatch will complain if you assign a negative value to x.

test.c:8 test() warn: assigning (-3) to unsigned variable 'x'

It's supposed to print a warning if you used it to save error codes like:

	x = some_kernel_function();

But it does not.  :/  Something to investigate.

regards,
dan carpenter