Re: [PATCH v2 2/2] fs: add the ability for statmount() to report the mount devicename

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2024-11-07 at 10:40 +0100, Jan Kara wrote:
> On Wed 06-11-24 14:53:06, Jeff Layton wrote:
> > /proc/self/mountinfo displays the devicename for the mount, but
> > statmount() doesn't yet have a way to return it. Add a new
> > STATMOUNT_MNT_DEVNAME flag, claim the 32-bit __spare1 field to hold the
> > offset into the str[] array. STATMOUNT_MNT_DEVNAME will only be set in
> > the return mask if there is a device string.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> Just one question below:
> 
> > @@ -5078,6 +5091,12 @@ static int statmount_string(struct kstatmount *s, u64 flag)
> >  		if (seq->count == sm->fs_subtype)
> >  			return 0;
> >  		break;
> > +	case STATMOUNT_MNT_DEVNAME:
> > +		sm->mnt_devname = seq->count;
> > +		ret = statmount_mnt_devname(s, seq);
> > +		if (seq->count == sm->mnt_devname)
> 
> Why this odd check? Why don't you rather do:
> 		if (ret)
> ?
>

statmount_mnt_devname() can return without emitting anything to the seq
if ->show_devname and r->mnt_devname are both NULL. In that case, we
don't want statmount_string() to return an error, but we also don't
want to do any further manipulation of the seq. So, the above handles
either the case where show_devname returned an error and the case where
there was nothing to emit.

I did consider having statmount_mnt_devname() return -ENOBUFS if there
was nothing to emit, and then handle that in the caller, but checking
to see whether the seq has advanced seemed like a cleaner solution.

I can add a comment to that effect since it does look a bit confusing.


> > +			return ret;
> > +		break;
> >  	default:
> >  		WARN_ON_ONCE(true);
> >  		return -EINVAL;
> 
> 								Honza
> 

-- 
Jeff Layton <jlayton@kernel.org>