On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > > the user to maintain backward compatibility.
> > > >
> > > > I wonder if this really helps. What do I do if I see a version that I
> > > > don't understand? Ignore the whole file? That doesn't make for a good
> > > > user experience.
> > >
> > > There is no UX consideration here. A user browsing the file directly
> > > will not care about the version.
> > >
> > > This file is intended to be parsable by scripts and they have to
> > > keep up with the occasional changes in format. Scripts can handle an
> > > unrecogized version however they like.
> > >
> > > This is what we typically get with a made-up format that isn't .ini
> > > or JSON or XML. The file format isn't self-documenting. The final
> > > field on each row is a variable number of tokens, so it will be
> > > nearly impossible to simply add another field without breaking
> > > something.
> > >
> >
> > It shouldn't be a variable number of tokens per line.
>
> That's how NFSv4 COMPOUND operations are displayed. For example:
>
> 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
>
> The list of operations in the displayed compound are currently
> blank-separated tokens at the end of each row.
>
Oh! That's a bug in missed in my latest review then. The operations
field was delimited by ':' chars at one point. Lorenzo, did you mean to
change that?
IMO, the list of operations should be one field, separated by a distinct
delimiter (like ':').
>
> > If there is, then that's a bug, IMO. We do want it to be simple to
> > just add a new field, published version info notwithstanding.
>
> They could be wrapped in curly braces, or separated by commas, to
> make them all one token.
>
> I haven't looked at NFSv3 output yet, but I expect those extra
> tokens won't even be there in that case.
>
That's probably another bug. Anything not a v4 COMPOUND should have
something as a placeholder. It could just be a single '-' character.
> JSON, yaml, or xml would all address the extensibility problem, just
> as an alternative thought.
>
It would probably be fairly simple to output well-formed yaml instead.
JSON and XML are a bit more of a pain.
For now, we can change the output. We do need to have this settled
before this goes to Linus' tree though.
--
Jeff Layton <[email protected]>
On Tue, Aug 08, 2023 at 10:20:44AM -0400, Jeff Layton wrote:
> On Tue, 2023-08-08 at 10:03 -0400, Chuck Lever wrote:
> > On Tue, Aug 08, 2023 at 09:48:42AM -0400, Jeff Layton wrote:
> > > On Tue, 2023-08-08 at 09:24 -0400, Chuck Lever wrote:
> > > > On Tue, Aug 08, 2023 at 09:33:23PM +1000, NeilBrown wrote:
> > > > > On Tue, 08 Aug 2023, Lorenzo Bianconi wrote:
> > > > > > Introduce version field to nfsd_rpc_status handler in order to help
> > > > > > the user to maintain backward compatibility.
> > > > >
> > > > > I wonder if this really helps. What do I do if I see a version that I
> > > > > don't understand? Ignore the whole file? That doesn't make for a good
> > > > > user experience.
> > > >
> > > > There is no UX consideration here. A user browsing the file directly
> > > > will not care about the version.
> > > >
> > > > This file is intended to be parsable by scripts and they have to
> > > > keep up with the occasional changes in format. Scripts can handle an
> > > > unrecogized version however they like.
> > > >
> > > > This is what we typically get with a made-up format that isn't .ini
> > > > or JSON or XML. The file format isn't self-documenting. The final
> > > > field on each row is a variable number of tokens, so it will be
> > > > nearly impossible to simply add another field without breaking
> > > > something.
> > > >
> > >
> > > It shouldn't be a variable number of tokens per line.
> >
> > That's how NFSv4 COMPOUND operations are displayed. For example:
> >
> > 0x5d58666f 0x000000d1 0x000186a3 NFSv4 COMPOUND 0000062034739371 192.168.103.67 0 192.168.103.56 20049 OP_SEQUENCE OP_PUTFH OP_READ
> >
> > The list of operations in the displayed compound are currently
> > blank-separated tokens at the end of each row.
> >
>
> Oh! That's a bug in missed in my latest review then. The operations
> field was delimited by ':' chars at one point. Lorenzo, did you mean to
> change that?
>
> IMO, the list of operations should be one field, separated by a distinct
> delimiter (like ':').
>
> >
> > > If there is, then that's a bug, IMO. We do want it to be simple to
> > > just add a new field, published version info notwithstanding.
> >
> > They could be wrapped in curly braces, or separated by commas, to
> > make them all one token.
> >
> > I haven't looked at NFSv3 output yet, but I expect those extra
> > tokens won't even be there in that case.
> >
>
> That's probably another bug. Anything not a v4 COMPOUND should have
> something as a placeholder. It could just be a single '-' character.
>
> > JSON, yaml, or xml would all address the extensibility problem, just
> > as an alternative thought.
> >
>
> It would probably be fairly simple to output well-formed yaml instead.
> JSON and XML are a bit more of a pain.
If folks don't mind, I would like more structured output like one of
these self-documenting formats. (I know I said I didn't care before,
but I'm beginning to care now ;-)
I'm also wondering if we really ought not add another file under
/proc, which is essentially obsolete. Would /sys/fs/nfsd/yada be
better for this facility?
I hesitate to even mention network namespaces...
> For now, we can change the output. We do need to have this settled
> before this goes to Linus' tree though.
Agreed. As fair warning, I might drop this from v6.6 if we need more
time to get it right. That doesn't mean I'm not excited about having
this facility available for all our users.
--
Chuck Lever