Hi Krzysztof,
thanks for reviewing!
[...]
> To me, this has a bit of code smell. The double pointer needs to be passed
> to the helper function if we want to acquire the file inside it. However
> we can also just let the caller handle that acquisition, especially since
> we have it call fput() anyway. That way it's clear that it's the caller
> who actually has to manage the resource during its lifetime. This will
> also allow the helper function to take just a copy of the file pointer,
> getting rid of all the redundant dereferencing inside.
>
> So IMO the usage should be more like:
>
> <in caller function>
>
> unsigned long addr;
> struct file *file;
>
> file = acquire_in_some_manner();
> addr = __igt_mmap_offset(i915, offset, size, prot, flags, file);
>
> /* do some operations */
>
> fput(file);
>
> </in caller function>
>
> > +unsigned long igt_mmap_offset_get_file(struct drm_i915_private *i915,
> > + u64 offset,
> > + unsigned long size,
> > + unsigned long prot,
> > + unsigned long flags,
> > + struct file **file)
> > +{
> > + return __igt_mmap_offset(i915, offset, size, prot, flags, file);
> > +}
>
> Also given the above I'd reconsider this function. Since we're now
> expecting the caller to acquire the file anyway, we could probably just
> put the body of __igt_mmap_offset() into igt_mmap_offset_get_file() and
> call _get_file() from igt_mmap_offset().
>
Great suggestions, haven't thought about it that way. I like the
last part especially - putting the get_file call inside
igt_mmap_offset().
Best Regards,
Krzysztof
