Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Anton,

On Saturday 29 March 2014 09:28:02 Anton Leontiev wrote:
> 28.03.2014 20:12, Laurent Pinchart пишет:
> >>>> + * Set error flag for incomplete buffer.
> >>>> + */
> >>>> +static void uvc_buffer_check_bytesused(const struct uvc_streaming
> >>>> *const stream,
> > 
> > No need for the second const keyword here.
> > 
> > I would have used "uvc_video_" as a prefix, to be in sync with the
> > surrounding functions. What would you think of
> > uvc_video_validate_buffer() ?
> > 
> >>>> +	struct uvc_buffer *const buf)
> > 
> > And no need for const at all here.
> > 
> >>>> +{
> >>>> +	if (buf->length != buf->bytesused &&
> >>>> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
> > 
> > The indentation is wrong here, the ! on the second line should be aligned
> > to the first 'buf' of the first line.
> > 
> > If you agree with these changes I can perform them while applying, there's
> > no need to resubmit the patch.
> 
> Thank you for reviewing my first patch to Linux kernel.

Thank you for submitting it :-)

> I completely agree with your changes.
> 
> Just want to ask why there is no need for the second 'const' after pointer
> character '*'? I thought it marks pointer itself as constant for type-
> checking opposite to first 'const', which marks memory it points to as
> constant for type-checking.

Your understanding is correct (as far as I know).

> I understand that the function is simple enough to verify it by hand but
> it's better to add more information for automatic checking.
>
> Is there any guidelines on 'const' keyword usage in Linux kernel code?

Marking the memory pointed to by the pointer as const ensures that the 
function won't modify memory that the caller thought wouldn't be modified. It 
thus serves as a contract between the caller and the callee, enforced by the 
compiler.

Marking the pointer as const, on the other hand, only affects the function 
implementation, without influencing its call contract. Its only use in this 
case would be to prevent the function from modifying a pointer that the 
function itself thought it shouldn't modify. While not useless in all cases, 
this compile-time checked if usually not performed in the kernel, especially 
for simple functions. I'm not aware of any explicit rule regarding const usage 
though.

-- 
Regards,

Laurent Pinchart