On Thu, Apr 25, 2024 at 09:25:50PM +0100, Al Viro wrote:
> On Thu, Apr 25, 2024 at 11:57:12AM +0200, Christian Brauner wrote:
> > On Thu, Apr 25, 2024 at 01:38:59AM +0200, Dawid Osuchowski wrote:
> > > Creates an anon_inode_getfile_fmode() function that works similarly to
> > > anon_inode_getfile() with the addition of being able to set the fmode
> > > member.
> >
> > And for what use-case exactly?
>
> There are several places where we might want that -
> arch/powerpc/platforms/pseries/papr-vpd.c:488: file = anon_inode_getfile("[papr-vpd]", &papr_vpd_handle_ops,
> fs/cachefiles/ondemand.c:233: file = anon_inode_getfile("[cachefiles]", &cachefiles_ondemand_fd_fops,
> fs/eventfd.c:412: file = anon_inode_getfile("[eventfd]", &eventfd_fops, ctx, flags);
> in addition to vfio example Dawid mentions, as well as a couple of
> borderline cases in
> virt/kvm/kvm_main.c:4404: file = anon_inode_getfile(name, &kvm_vcpu_stats_fops, vcpu, O_RDONLY);
> virt/kvm/kvm_main.c:5092: file = anon_inode_getfile("kvm-vm-stats",
>
> So something of that sort is probably a good idea. Said that,
Ok. It wouldn't be the worst if @Dawid would also sent a follow-patch
converting the obvious cases over to the new helper then.
> what the hell is __anon_inode_getfile_fmode() for? It's identical
Unrelated to this patch but the other really unpleasant part is that
"make_secure" boolean argument to __anon_inode_getfile() to indicate
allocation of a new inode. That "context_inode" argument is also really
unused for the normal case where the same anon_inode_inode is reused...
IMHO, that should just be split apart. A little more code but it would
make things easier to understand imho...
> to exported variant, AFAICS. And then there's this:
>
> if (IS_ERR(file))
> goto err;
>
> file->f_mode |= f_mode;
>
> return file;
>
> err:
> return file;
>
> a really odd way to spell
>
> if (!IS_ERR(file))
> file->f_mode |= f_mode;
> return file;
Yeah.
