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 12:57 +0100, Jan Kara wrote:
> On Thu 07-11-24 06:15:40, Jeff Layton wrote:
> > 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.
> 
> OK, but don't we want to emit an empty string - i.e., just \0 character in
> case r->mnt_devname is NULL and there's no ->show_devname?
> 

Yes. I think it's better to just not set STATMOUNT_MNT_DEVNAME at all
if it's not present than to emit an empty string.

> Because now we
> literaly emit nothing and it's going to be very confusing for userspace to
> parse this when this happens in the middle of other strings in the seq.
> And emitting \0 is exactly what will happen if we don't do anything special
> in STATMOUNT_MNT_DEVNAME case...
> 

That's why I'm having statmount_string() return immediately if no
string was emitted. That avoids the setting of the flag in the mask and
the unneeded NULL termination in the buffer.

The caller will need to check whether STATMOUNT_MNT_DEVNAME is set in
the returned mask. If it's not set, then no string was emitted and the
sm->mnt_devname index is invalid. In that case, we will have emitted
nothing into the buffer, so the other strings shouldn't be affected.

FWIW, here's the comment I added over that if statement:

+               /*
+                * If nothing was emitted, return immediately to avoid
+                * setting the flag and NULL terminating the buffer.
+                */
+               if (seq->count == sm->mnt_devname)
+                       return ret;

-- 
Jeff Layton <jlayton@kernel.org>