On Mon, Feb 07, 2022 at 04:01:01PM +0000, Al Viro wrote:
> On Mon, Feb 07, 2022 at 12:17:11AM -0300, Leonardo Araujo wrote:
> > From: "Leonardo Araujo" <leonardo.aa88@xxxxxxxxx>
> >
> > WARNING: struct file_operations should normally be const
> >
> > Signed-off-by: Leonardo Araujo <leonardo.aa88@xxxxxxxxx>
> > ---
> > drivers/staging/android/ashmem.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> > index ddbde3f8430e..4c6b420fbf4d 100644
> > --- a/drivers/staging/android/ashmem.c
> > +++ b/drivers/staging/android/ashmem.c
> > @@ -377,7 +377,7 @@ ashmem_vmfile_get_unmapped_area(struct file *file, unsigned long addr,
> >
> > static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> > {
> > - static struct file_operations vmfile_fops;
> > + static const struct file_operations vmfile_fops;
> > struct ashmem_area *asma = file->private_data;
> > int ret = 0;
>
> Wait a minute. Why the hell would it possibly want a private instance
> of all-NULLs file_operations? Odd...
> <checks>
> if (!vmfile_fops.mmap) {
> vmfile_fops = *vmfile->f_op;
> vmfile_fops.mmap = ashmem_vmfile_mmap;
> vmfile_fops.get_unmapped_area =
> ashmem_vmfile_get_unmapped_area;
> }
> Er... So it *is* modified down the road. What, in your opinion, is signified
> by the const you are adding?
>
> Folks, could we please have the first "WARNING" in checkpatch.pl output replaced
> with
> "I'm a dumb script; this line looks like there might be something fishy in the
> area. Somebody smarter than me might want to take a look and figure out if
> there's something wrong going on there. From now on I'll mark all such places
> with 'WARNING' (with the summary of heuristics that pointed to them), to avoid
> repeating the above".
>
> Pretty please? This exact trap keeps snagging newbies - folks misinterpret
> "this place might be worth looking into" for "great (s)tool says: this is
> what's wrong there; must propitiate the great (s)tool!"
>
> In this case the damage is minimal - the resulting change would be instantly
> caught by compiler, so it's just a matter of mild embarrassment for poster.
> In other cases results had been nowhere near as mild.
It's a great "catch" for people who try to modify the kernel and then
never actually test-build their changes. So for that reason alone I
like it as it does reinforce the need for drive-by people to at least
build the kernel tree.
> Incidentally, the place those heuristics had pointed too _DOES_ look fishy,
> indeed. What happens, AFAICS, is that the first time we hit that branch
> (asma->file being NULL) we stash a copy of whatever file_operations we get
> on file obtained by shmem_setup_file() (IOW, shmem_file_operations),
> with ->mmap and ->get_unmapped_area replaced with local functions.
> This is a bloody convoluted way to do things, not to mention being rather
> brittle...
>
Ashmem is horrid and is not used by the Android project anymore, except
by really old userspace programs. I think we could just delete it
entirely now, I'll go ask the Android developers about it.
thanks,
greg k-h
