Added a more descriptive error message due the destroy failure

[Artem-OSSRevival]

Motivation and Context

Change improves interaction with the user. Makes an error message more understandable and descriptive.
#14538

Description

Previously, the destroy failure due to a long hold returned a standard error message: dataset is busy
This could cause misunderstanding and confusion among users.
The change makes the error message more understandable and definite, displaying the presence of the holder.

This is implemented by performing an additional check for the presence of holders.
If any, displayed: held by "holder" instead of dataset is busy

Before:

sudo zfs snapshot rpool@test
sudo zfs hold example rpool@test
sudo zfs destroy rpool@test
cannot destroy snapshot rpool@test: dataset is busy
sudo zfs release example rpool@test
sudo zfs destroy rpool@test

After:

sudo zfs snapshot rpool@test
sudo zfs hold example rpool@test
sudo zfs destroy rpool@test
cannot destroy snapshot rpool@test: held by example
sudo zfs release example rpool@test
sudo zfs destroy rpool@test

How Has This Been Tested?

Run the commands in the different combinations.

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)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

More detailed errors are always great, but I don't very like that additional details are not obtained atomically from the original error, but fetched separately, so they might be out of sync, theoretically confusing.

[amotin]

Also seems it failed to build:

   CC       lib/libzfs/libzfs_la-libzfs_import.lo
  lib/libzfs/libzfs_dataset.c: In function 'zfs_destroy_snaps_nvl':
  lib/libzfs/libzfs_dataset.c:4032:4: error: a label can only be part of a statement and a declaration is not a statement
      nvlist_t *existing_holds;
      ^~~~~~~~

Or in Clang:

  lib/libzfs/libzfs_dataset.c:4032:4: error: label followed by a declaration is a C23 extension [-Werror,-Wc23-extensions]
   4032 |                         nvlist_t *existing_holds;
        |                         ^
  1 error generated.

[Artem-OSSRevival]

Sorry, I did not notice this problem. Everything went smoothly on my device.

Perhaps other compiler requires that the nvlist_t *existing_holds declaration should be outside switch...case or something. I'll try to figure it out.

[AttilaFueloep]

I agree with amotin about the non-atomicity of the change. Still I think this is a nice improvement, pointing at the hold will benefit users. On the net you'll find a lot of question asked by users confused by the fact they couldn't delete seemingly unused datasets.

As a side note, in this case I think it's OK not to indent the block contents since that would harm readability. What does checkstyle say, by the way?

[Artem-OSSRevival]

On my side, the checkstyle was successful.
If indent the block, then it will not fit into the permissible length of the line.

[amotin]

@Artem-OSSRevival If you search for other examples, more typical is to open the braces at the end of case line and close on the line after break, aligned to the next case. This way you don't need additional indentation.

[amotin]

Not exactly. Just search for ': {'.

[Artem-OSSRevival]

I understood correctly? I can now fix the commit.

[AttilaFueloep]

Yeah, I think this is the best solution.

[AttilaFueloep]

Still looks good.

[AttilaFueloep]

To summarize, as initially written it required C23, starting with C99 case X:; would do the trick, and as it is now, it works from C89 onwards.

[mcmilk]

The two commits should be merged.

[amotin]

Looking again, all other callers of lzc_get_holds() check for possible errors. Without it you'll likely get some uninitialized memory dereference in nvlist_empty() below.