Remove inappropiate error message suggesting to apply '-r'

[InsanePrawn]

Remove inappropiate error message suggesting to use '-r'

Removes an incorrect error message from libzfs that suggests applying
'-r' when a zfs subcommand is called with a filesystem path while
expecting either a snapshot or bookmark path.

Signed-off-by: InsanePrawn [email protected]

Removes an additional error message suggesting to append '-r' to the zfs command in case zfs_validate_name expects either a bookmark or a snapshot and gets a snapshot/bookmark without the '@'/'#'.

Motivation and Context

#8352 introduced more detailed error messages when handling dataset paths. Two of them recommend -r when it expects a snapshot/bookmark path but finds no '@'/'#' symbol. (lib/libzfs/libzfs_dataset.c, currently line 123).

$ sudo zfs clone yolopool/data yolopool/data
cannot open 'yolopool/data': missing '@' delimiter in snapshot name, did you mean to use -r?
$ sudo zfs diff yolopool/data
Badly formed snapshot name yolopool/data: missing '@' delimiter in snapshot name, did you mean to use -r?

This message seems to appear anywhere either a snapshot or bookmark path is expected but a dataset path is provided by the user. Not all commands that handle snapshots support -r and i doubt we should be blindly recommending it even if they did ;)

In fact, after some staring at man pages and grepping through the repo, I was not able to identify a single case where the -r recommendation can come up and makes sense, although I'm not very familiar with the code base and could easily have missed something.
Either way, I suspect zfs_validate_name is not the right place to recommend command options. 🙃

Description

Removed the -r suggestion, leaving the information about expecting a snapshot/bookmark but not finding the respective separator '@'/'#'.

$ sudo zfs diff yolopool/data
Badly formed snapshot name yolopool/data: missing '@' delimiter in snapshot name

How Has This Been Tested?

Compiled it on Debian Stretch, confirmed the new error messages match the expected result.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.

[PrivatePuffin]

These kind of advices should indeed only be given for things that can be described as "The usual mistakes".
Changes are clear and obviously error free...

[codecov]

Codecov Report

Merging #9574 into master will decrease coverage by 12.53%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #9574       +/-   ##
===========================================
- Coverage   78.95%   66.42%   -12.54%     
===========================================
  Files         418      336       -82     
  Lines      123681   106209    -17472     
===========================================
- Hits        97653    70546    -27107     
- Misses      26028    35663     +9635

Flag	Coverage Δ
#kernel	?
#user	66.42% <ø> (-0.08%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 035ebb3...4c43cd4. Read the comment docs.

[behlendorf]

@madwizard would you mind reviewing this small proposed change.

[PrivatePuffin]

I did some additional research we should look into:

In contrast to what @InsanePrawn was saying, this issue was not introduced in #8352 but in #8047, because of the request in #7056

Which means another solution should be found for #7056 if we merge this.

This also answers @InsanePrawn 's question:

I was not able to identify a single case where the -r recommendation can come up and makes sense, although I'm not very familiar with the code base and could easily have missed something.

This also means @behlendorf that this issue does not originate with the refactor by @madwizard in #8352 but in the changes in #8047

[InsanePrawn]

Ah, while it's nice that you found the place where -r makes sense, I stand with my two comments:

I was not able to identify a single case where the -r recommendation can come up and makes sense

the and should probably be capitalised. zfs list doesn't seem to have a call path that calls zfs_validate_name() with ZFS_TYPE_SNAPSHOT (note the == in zfs_validate_name()), so the -r error message never actually shows up

# dataset with snapshots
$ sudo zfs list -tsnap poolparty
NAME                        USED  AVAIL     REFER  MOUNTPOINT
poolparty@PRE_0.8_INSTALL     0B      -      140K  -

# dataset without snapshots
$ sudo zfs list -tsnap poolparty/abc
no datasets available

# nonexistent dataset
$ sudo zfs list -tsnap poolparty/abc/d
cannot open 'poolparty/abc/d': dataset does not exist

Now, for bookmarks, we can actually get the old message:

$ sudo zfs list -tbookmark poolparty
cannot open 'poolparty': missing '#' delimiter in bookmark name, did you mean to use -r?

But this brings me to my second important statement:

Either way, I suspect zfs_validate_name is not the right place to recommend command options.

This additional, command-specific error message should IMHO be in the command function (zfs_do_list), not the widely used zfs_validate_name().
Actually, today I found out Tom Caputi already made these exact observations in #8352 (review)
Maybe we should use this opportunity to also look into his other concern :D

[PrivatePuffin]

@InsanePrawn it was not meant as an argument to devalue your PR.
I still stand with my approval and comment too:
"These kind of advices should indeed only be given for things that can be described as "The usual mistakes"."

edit
And yes, I agree this might be a good oppertunity to handle to observations by mister Caputi too. Fix it good, once and for all.

[InsanePrawn]

@InsanePrawn it was not meant as an argument to devalue your PR.

No hard feelings implied, 'while it's nice' was not sarcastic either. I was genuinely happy to see you find the intended usage :)

"These kind of advices should indeed only be given for things that can be described as "The usual mistakes"."

While I would argue that expecting to list snapshots with zfs list -tsnap pool/abc instead of zfs list -tsnap -d1 pool/abc counts as 'the usual mistakes' and warrants a hint, this isn't the behaviour we're seeing (anymore?); zfs list -tsnap poolparty "just works" (see above).

Weirdly, this doesn't work for bookmarks, as demonstrated above. Instead of reintroducing a -r hint for bookmarks, I'd argue in favour of aligning the behaviour of zfs list for bookmarks with the behaviour for snapshots instead.

[PrivatePuffin]

@InsanePrawn
I think we are generally on the same page, with different choice of words 👍

[behlendorf]

This isn't the behaviour we're seeing (anymore?);

That's right, @tcaputi was nice enough to add this functionality in #8539. I think extending it to bookmarks would make a lot of sense if someone wants to tackle that improvement, see df58307.

I'm also all for dropping this increasingly confusing hint from the common code. So let's merged this PR as is, we can always follow it up with additional improvements.