Add recursive/limited -a mount & unmount | introduce -A for noauto bypass

[QORTEC]

Note:
This post was rewritten/updated.

Motivation and Context

This pull request is intended to simplify the mounting and unmounting of the desired filesystems, resolving #2901 (recursive mounting).

This PR modifies the mount and unmount commands by adding an optional filesystem argument after the -a flag. This allows the user to specify a filesystem, and the command will mount/unmount the specified filesystem and its children, provided they are 'available'.
The code for recursive/limited mounting uses the -a code/logic paths. In practice, this means that the user can specify any valid filesystem, even those with the canmount=off property. The code will then run the standard -a code with the additional limitation that the dataset must be the specified filesystem or its children.

This PR also introduces the -A flag with the goal of simplifying the mounting and unmounting of datasets with the canmount=noauto property. The -A flag is a modified version of -a, and the code uses the same code paths that -a uses. However, in this case, we bypass the canmount=noauto check, resulting in file filesystems being treated as canmount=yes. To maintain the expected behavior, that datasets with canmount=noauto should not be automatically mounted unless explicitly specified, the -A flag requires a filesystem to be specified.

Description

I implemented recursive/limited mounting by leveraging the zfs mount -a code that was already present:

• share_mount() code is called by zfs mount
  • do_all was originally an int but was used as a true or false switch, so I changed it into a more appropriate boolean_t
  • I introduced boolean_t do_noauto, which is linked to the -A flag (for mounting datasets with noauto)
  • I introduced const char *filesystem, which takes input from argv[0]
  • We validate that filesystem is a valid ZFS filesystem by using zfs_open()
  • Then we pass the filesystem to get_all_datasets() using the parent parameter
• get_all_datasets() returns a list of datasets (to mount)
  • The const char *parent parameter was added, and it's the value passed to ga_parent in the get_all_state_t struct
  • zfs_iter_root() iterates over root datasets, and pass them to get_one_dataset()
• get_all_state_t struct is used to pass arguments to get_one_dataset()
  • const char *ga_parent was added; it's used for the parent dataset to match against
• get_one_dataset() iterates over and returns valid datasets (to mount)
  • We added an extra check: if ga_parent has a value, we pass the value to zfs_related_dataset() to check if the datasets are related
    • If the dataset is related, then continue
    • If the dataset is not related, then proceed to the next iteration
• share_mount() At this point, we have a list of datasets that start with or is filesystem itself
  • Next, we pass the value of do_noauto to sm_mount_noauto in the share_mount_state_t struct
  • share_mount_one_cb() is called to mount the list of datasets
• share_mount_state_t struct is used to pass arguments to share_mount_one_cb()
  • boolean_t sm_mount_noauto was added; it's used to treat canmount=noauto as canmount=on
• share_mount_one_cb() is a callback for mounting filesystems
  • It calls share_mount_one() and passes the value sm_mount_noauto to the mount_noauto property
• share_mount_one() is responsible for mounting filesystems
  • boolean_t mount_noauto was added so we can override the canmount check to treat canmount=noauto as canmount=on
  • The canmount check was modified to include mount_noauto
    • If mount_noauto is false, use normal logic
    • If mount_noauto is true, treat it as an explicit mount/canmount=yes

I implemented recursive/limited unmounting by leveraging the zfs unmount -a code that was already present:

• unshare_unmount() code is called by zfs unmount
  • do_all was originally an int but was used as a true or false switch, so I changed it into a more appropriate boolean_t
  • I introduced boolean_t do_noauto, which is linked to the -A flag (for mounting datasets with noauto)
  • I introduced const char *filesystem, which takes input from argv[0]
  • We validate that filesystem is a valid ZFS filesystem by using zfs_open()
  • The canmount check was modified to include mount_noauto
    • If mount_noauto is false, use normal logic
    • If mount_noauto is true, treat it as an mount/canmount=yes
  • We added an extra check: if filesystem has a value, we pass the value to zfs_related_dataset() to check if the datasets are related
    • If the dataset is related, then continue
    • If the dataset is not related, then proceed to the next iteration

libzfs.h

---

Added the following functions:

• related_dataset(): Checks if the datasets are the same or if the second one is a child
  • Check if both datasets are the same
    • If true: (return B_TRUE)
  • Check if it is a child dataset using is_descendant()
    • If true: (return B_TRUE)
  • (return B_FALSE)
• zfs_related_dataset(): A helper function for related_dataset()

libzfs_dataset.c

---

zfs-mount(8):

zfs mount
 -a filesystem
     Mount the specified filesystem and its children, provided they are available.
     If no filesystem is specified, then all available ZFS file systems are mounted; may be invoked automatically as part of the boot process if configured.
 -A filesystem
     Mount the specified filesystem and its children, provided they are available.
     Note: datasets with the canmount=noauto property will also be mounted.

zfs unmount
 -a filesystem
     Unmount the specified filesystem and its children, provided they are available.
     If no filesystem is specified, then all available ZFS file systems are unmounted. Invoked automatically as part of the shutdown process.
 -A filesystem
     Unmount the specified filesystem and its children, provided they are available.
     Note: datasets with the canmount=noauto property will also be unmounted.

---

Note:
This pull-request should be carefully reviewed before it's accepted:

• I AM NOT a c programmer; this is the first c project that I've worked on.
• I am using ChatGPT to:
  • Get/verify my understanding of what the code is doing.
  • Request specific code examples, such as:
    • "compare two strings"
    • "zfs function that retrieves the dataset name"
  • Review code, looking for errors.
• I do interrogate ChatGPT about any results that I don't understand.

I feel like I have a basic/limited understanding of the ZFS code I've touched and have made an effort to keep my changes as simple and low-impact as possible.
The implemented code is simple and easy to understand. However, it is important to note that since I am not a C developer, and that my code should be carefully reviewed for common beginner-level mistakes, such as memory management.

In its current state, this PR should be ready for review.

How Has This Been Tested?

I mounted and unmounted datasets on a test PC running Linux.

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.

[ahesford]

Wow. If somebody with no programming knowledge submitted an AI-generated PR on one of my repos, I'd ban the person for life.

Offer fixes and features you've thoughtfully developed, not that you shoved into a chatbot.

[endgame]

As much as LLMs worry me, @QORTEC seems to have used them to gain an understanding of the code and asked for specific low-level coding advice like "compare two strings". This seems like the class of question which would be perfectly reasonable to search for on StackOverflow or other help sites.

I think it would be a different story if @QORTEC had generated the patch wholesale. Also, extreme negative reactions are just going to make people hide their LLM usage; I think @QORTEC definitely did the right thing here by explicitly calling it out as a good-faith attempt and his best effort, so that the PR could be reviewed thoroughly.

[endgame]

I know C but I don't work on OpenZFS. HTH.

[ahesford]

I certainly appreciate that the submitter was honest about relying on ChatGPT, but the statements "I do not know C", "I used ChatGPT to get a[n] understanding", "I requested specific code", and "I had ChatGPT review the finalized code" are functionally equivalent to deferring all judgment about the changeset to a large language model.

Even in a piece of software less important than a filesystem people rely on for robust data storage, deferring judgment about patches to a system devoid of sentience or an understanding of the domain, with a known history of complete fabrication, is undesirable. Contributions of this nature can sap project resources as more effort must be expended on review and correction. Blind submissions of this nature should be discouraged; those without a thorough understanding of the code they are touching, or even of the language in which it's written, should instead be encouraged to contribute in more meaningful ways. Even a feature-request issue with a well-formed description of desired behavior is more helpful than copying and pasting code snippets.

It would be almost as bad to submit PRs consisting of functions naively copied from StackOverflow.

[ahrens]

@ahesford

Contributions of this nature can sap project resources as more effort must be expended on review and correction.

You are free to engage with the OpenZFS project as much or as little as you like, including not expending your resources on reviewing this PR. Please allow others to contribute as they would like, including reviewing this PR and helping the submitter to improve it. Please do not attempt to speak on the behalf of the project to discourage others contributions.

[ahesford]

I never attempted to speak on behalf of this project. To the contrary, I highlighted the independence of my opinion by noting how I would react to similar submissions on my own repositories. It is also abundantly clear from the badges on my comments that I am an observer rather than a member of the organization.

My initial and subsequent comments were intended as a (harshly) negative review, suggesting that this class of code contributions has substantial drawback. If you believe these comments are inappropriate, you of course have editorial authority.

[ahrens]

@QORTEC Thank you for working to improve OpenZFS! I hope you'll take a look at the code review comments made by @endgame and @lundman and continue refining this PR.

I've created the separate discussion topic #15269 to share my thoughts on AI-assisted contributions.

[kgraaf]

While this is a relatively simple and minor set of changes, the very nature of open source development clearly relies on the baseline assumption that anyone submitting PRs generally knows what they're doing on some level. The creator of this one, by their very own admission, does not. It's pretty difficult for me to see submissions like this one as being made in a good faith effort: there is no shortage of materials on learning C (of all languages) which the creator appears to have made no effort whatsoever to engage with. "Judging PRs by their merits" sounds nice, but are maintainers of open source projects now supposed to assume that every contribution potentially contain machine-generated sludge with no thought put into it? I think that says little good about the spirit of collaboration.

Programming languages are, at their essence, tools of thought, and thinking cannot be delegated to text prediction models.

[endgame]

open source development clearly relies on the baseline assumption that anyone submitting PRs generally knows what they're doing on some level. The creator of this one, by their very own admission, does not.

I started from the assumption that OP was a programmer, but not a C programmer, and from there assumed familiarity with the execution model of imperative languages but not necessarily the C libraries. I'm not sure why other commenters did not. I think the careful formatting of the opening statement led me to form this view, though that can of course be generated by an LLM in seconds.

[ahesford]

Sigh. Let me start with a substantive criticism of the change submitted and add a few more general comments after.

1. The fundamental logic in this PR is fatally flawed. The parent dataset taken as input is completely unsanitized (not to mention foolishly assigned: const char *dataset = (argv[0] != NULL) ? argv[0] : NULL; is a really goofy way to just say const char *dataset = arvg[0]). Later, that unsanitized input is used as a prefix against which to match the list of all datasets. The prefix doesn't even have to be a complete dataset name, and the check is completely divorced from the actual hierarchy! Specifying a dataset argument of pool/a will match pool/a/b as well as pool/another, contrary to the spirit of the addition. It even looks like specifying an argument of p will match pool1 as well as pool2.
   [Note: this assumes that zfs_get_name even returns a fully qualified path and not just the name of that node, e.g., returning child for pool/parent/child. I haven't looked closely enough to be sure.]
2. The added inline help is fundamentally wrong and also breaks convention with respect to the difference between [optional] and <required> parameters.

The first issue is a critical flaw that should have disqualified this submission out of the gate. Instead, an uniformed submission has caused people to devote valuable time to understanding a changeset that doesn't even do what it says.

Whether or not the submitter is a programmer, it was declared up front that "I feel like I basic/limited have a understanding on the code I touched", which runs counter to the expression of a "baseline assumption that anyone submitting PRs generally knows what they're doing on some level" by @kgraaf as well as the sentiment expressed by @ahrens in #15269: "More importantly, we would like for submitters to have the time and understanding to participate in the code review process, and ideally to be around to help with problems that may arise with their code post-integration." Both of these statements are reasonable and I agree with each of them.

While there is nothing wrong with consulting "AI" as a reference by somebody with a modicum of understanding about the patch being submitted, that is very different than what has been done here.

[endgame]

Thanks for the thorough explanation.

[QORTEC]

@QORTEC seems to have used them [LLMs] to gain an understanding of the code and asked for specific low-level coding advice like "compare two strings".

I think it would be a different story if @QORTEC had generated the patch wholesale

thinking cannot be delegated to text prediction models

The issue with LLMs is that they don't think, so its is on us (users of LLMs) to do the thinking for them, and then decide if the output is valid or not;

• If you give an LLM a broad question like to "implement recursive mounting in ZFS", it will give you tons of useless sludge.
• If you point it at the correct source code saying "implement recursive mounting in ZFS", it will attempt to do so, but even if is successful it probably break you source.
• If you ask it to "compare two strings" now that's something LLMs can easily answer much more quickly and accurately.
• A separate issue being that LLMs wont always choose the correct (being the most simplest or efficient) method.

statements [... are] functionally equivalent to deferring all judgment about the changeset to a large language model

I think this is fair reaction to those statements.

there is no shortage of materials on learning C (of all languages) which the creator appears to have made no effort whatsoever to engage with

[open source development has] baseline assumption that anyone submitting PRs generally knows what they're doing on some level

Fair, though it's not so much as I never took a look at c because I did. It was difficult to get into, and in the end I decided use other languages with a lower bar to entry.
I find it difficult to learn in a structured programming class when I have no experience in the language, but it's also difficult to learn on my own (with only reference material) when there any where/one to ask questions. My approach to learning new languages is to work towards a task, and learning on they way what is needed to accomplish said task.
I personally don't like to claim knowledge when I don't feel like I can defend said knowledge, and everything I know when it comes to said languages tends to be narrow in scope.

I started from the assumption that OP was a programmer, but not a C programmer

I think the careful formatting of the opening statement led me to form this view, though that can of course be generated by an LLM in seconds.

If I must claim knowledge;

• I have taken a Visual Basic class, in which I didn't really learn anything.
• I also had another class with a scripting language, but I don't even remember what language it was.
• BASH, use it for most tasks I don't want to do my self...
• python, it's an easy language to get into, and that I'm most proficient in (after BASH)

Of course I've used/played with others, but probably only enought to claim knowledge on then their syntax and some basic commands.

Even a feature-request issue with a well-formed description of desired behavior is more helpful than copying and pasting code snippets.

Issue #2901, is almost a decade old and you'll notice it has been buried and no one is really interested in working on it.

suggesting that this class of code contributions has substantial drawback

are maintainers of open source projects now supposed to assume that every contribution potentially contain machine-generated sludge with no thought put into it?

extreme negative reactions are just going to make people hide their LLM usage

I've created the separate discussion topic #15269 to share my thoughts on AI-assisted contributions.

I think this is an important issue to discuss, and perhaps it's over due.

My initial and subsequent comments were intended as a (harshly) negative review

Honestly, besides you initial comment which feels like it was probably a knee jerk reaction, I think that your reviews while harsh are fair, and you explained your options/thoughts well.

@QORTEC Thank you for working to improve OpenZFS! I hope you'll take a look at the code review comments made by [@]endgame and [@]lundman and continue refining this PR.

Thank's for the kind words

---

In closing the purpose of this PR was to bring attention to a decade old issue #2901, and to say "Hay, at least I tried. Is my implementation any good?"
The reason I specifically called out my lack of confidence the way I did, was because I know you can do some really stupid stuff in c and I don't know if I'm guilty if it.
Personally, at best I would equate my level of c understanding to be at a Introduction to C level.

Also the fact that AI is becoming easier to access (like most new technology), makes it possible for people with "less" knowledge to work on a level that is beyond their personal skill set.
How ZFS chooses to treat AI, and AI assisted submissions is an important discussion that needs to happen.

No, I didn't use any AI for this response only spell check

Quotes from:
@ahesford @endgame @kgraaf @ahrens

[QORTEC]

Hay, at least I tried. Is my implementation any good?

If the answer is no, that that's fine and I'll happily close this pull request.
But if the answer is it just needs fixing then I'm happy work on it, and get it into a state where its acceptable to be included in OpenZFS.

[ahesford]

I'll defer to people with an understanding of ZFS internals to suggest the best method for checking lineage of a filesystem, but if string comparisons on the output of zfs_get_name are sufficient, the following changes should be made to make sure that the added features perform as expected:

• All references to dataset should be changed to filesystem; share operations only act on filesystems (at least as documented), and recursive mount operations only make sense for filesystems. (While you can certainly mount a snapshot, a snapshot can't be regarded as the parent of anything mountable.) This is really just a matter of clarity.
• When the dataset (now filesystem) parameter is taken from the command line, it should be validated by ensuring that it does not end in a trailing slash and that it actually names an existing filesystem. If these conditions fail, a meaningful error message should be reported rather than further attempting to walk the tree.
• A failed strncmp comparison is sufficient to reject a filesystem during the mount/share walk, but a successful comparison is not sufficient to accept it. The filesystem being tested against the parent should have a name that is either wholly identical to the parent or whose first character after the matching prefix is a slash.
• The help text and manual should be fixed to properly describe optional or required parameters. Something like

  usage:
  	mount
  	mount [-flvO] [-o options] -a [filesystem]
  	mount [-flvrO] [-o options] filesystem
  ```
  would be a bit more clear that `r` and `a` are mutually exclusive and that the `filesystem` parameter is only optional when `-a` is specified, but there may be a more concise way to describe these concepts.
  

• These recursive options also apply to zfs share, so the usage and manual page for that command should be similarly modified.

[ahesford]

I'll offer some suggestions subject to the caveats that they should be reconciled with whatever stylistic expectations the project expects, and also that you should seek counsel about whether there are more appropriate ways to test the parentage of a filesystem than string comparisons. I would think some libzfs routine exists to do this check in a more robust way.

Also, are you actually testing these changes locally?

[ahesford]

Is there a reason this is limited only to mount?

[QORTEC]

Yes/no, this pull request was originally for mount only, and zfs share has additional args [smb|nfs] that I didn't see where it was pulled from.
I thought it best to limit the PR to zfs mount untill/unless I look into how share works.

[ahesford]

There are no additional arguments documented for zfs share. The control flow is exactly the same except for the part that actually acts on each filesystem, so limiting this to mount-only seems arbitrary and causes an unnecessary split in the nature of the two recursive filesystem operations.

[QORTEC]

Agreed, I didn't see anything in the manpages, but usage differs?
Hence why I disabled it until I can investigate it properly.

[ahesford]

By the time you're consuming this argument, any nfs or smb argument has already been consumed zfs zfs share, so the syntax for zfs share becomes

zfs share [-l] <filesystem | <-a|-r> [<nfs|smb> [filesystem]]>

assuming, of course, that you don't drop the -r.

[QORTEC]

-r is not currently enabled for zfs share, I was using op == OP_MOUNT to restrict -a [filesystem].

I believe this would be the correct syntax for the -a arg;

zfs share [-l] <-a [nfs|smb] [filesystem] | filesystem>

My original reasoning for disabling -a [filesystem]

• I didn't have time to track down how the args work to verify if my usage is correct
• I would need to update the man pages adding the missing information [nfs|smb]
• I haven't use zfs share (something I should do when I have spare time)

[QORTEC]

Also, are you actually testing these changes locally?

Yes, though I still need to create a test script with the intention of breaking my implementation to ensure all works as expected.

[QORTEC]

Just got some time to work on my PR again. I've cleaned up the implementation based on the feedback received, and noted a couple of changes as well the reasoning behind them below:

---

-r is a sort of weird -a that behaves exactly as -a except that it ignores canmount=noauto and mounts it anyway.

I'd submit that the whole notion of -r is ill-formed, and you should abandon the new argument and stick to just adding the option for limiting the scope of -a to some subtree.

You make a good point that there are certain expectations regarding what recursive behavior should entail, and my implementation may not align with them. I made a poor choice in using -r and recursive as my flag and descriptors.

Originally, the intention was for -r filesystem to act exactly the same as -a filesystem, except it would also mount canmount=noauto filesystems. To better reflect this operation, I've chosen to use the -A flag to signify that it is a modified implementation of -a.

Additionally, I've decided to use mount_noauto instead of recursive as it more accurately describes the code implementation/intention.

The manpage for zfs mount also needed to be updated to align with the code implementation:

- Mounts the specified ZFS file system and its children.
+ Mounts the specified filesystem and its children, provided they are available.

The original wording, which was used for both -a and -r, falsely implied explicit recursive mounting. The updated wording should allow for a usage where you can execute zfs mount -a zpool/dataset, and if zpool/dataset has canmount=off the result will be the mounting of the filesystem's children instead of returning an explicit error.

---

The right place for that check is not here, because it's changing the notional behavior of get_one_filesystem

you'll need to separate the logic between explicit and recursive so you can properly ignore canmount=off for recursive operations as well.

I've updated the get_one_dataset() code so that it no longer includes the canmount=noauto check. Instead, I've added the mount_noauto property to share_mount_one(). This change allows us to modify the ZFS_CANMOUNT_NOAUTO check by adding !mount_noauto, which enables us to treat canmount=noauto as if it were canmount=yes.

---

Regarding zfs share; currently, the pull request does not enable <-a [nfs|smb] [filesystem] | [-A] [nfs|smb] filesystem>. My original reasoning:

• need to track down the args code to verify if my usage is correct
• need to update the manpages with information regarding [nfs|smb] (since it's currently missing).
• need to allocate spare time to verify that the code functions as expected

/* Limit `-a filesystem` to mount only */
if (op == OP_MOUNT)
    filesystem = argv[0];

[QORTEC]

Failing Tests

# DESCRIPTION:
#	Try each 'zfs mount' with inapplicable scenarios to make sure
#	it returns an error. include:
#		* '-a', but also with a specific filesystem.

set -A args "$mountall $TESTPOOL/$TESTFS"

typeset -i i=0
while (( i < ${#args[*]} )); do
	log_mustnot zfs ${args[i]}
	((i = i + 1))
done

zfs_mount_009_neg.ksh

# DESCRIPTION:
# Verify that zfs mount should fail with bad parameters

set -A badargs "A" "-A" "-" "-x" "-?" "=" "-o *" "-a"

for arg in "${badargs[@]}"; do
	log_mustnot eval "zfs mount $arg $fs >/dev/null 2>&1"
done

zfs_mount_011_neg.ksh

It looks like the tests are failing because:

• both tests seem to be executing zfs mount -a zpool/dataset and are expecting an error return
• zfs_mount_011_neg is executing zfs mount -A zpool/dataset, which it is also expecting to fail

The following tests will probably need to be added:

• -a zroot/dataset
  • ensure that zroot/dataset and its children are mounted (if allowed)
  • ensure that children of zroot/dataset are mounted (if zroot/dataset is not allowed to be mounted)
  • ensure we don't mount datasets based on partial name match (zroot/data != zroot/dataset)
• -A zroot/dataset
  • functions the same as -a except it treats canmount=noauto as canmount=yes

Note to self:

• tab indentation is 8 spaces not 4

[ahesford]

My initial rebuke of your contribution was based on my view that it is deeply unethical to submit project changes for consideration when you lack understanding of the contribution and are incapable of defending your submission in a thoughtful review process. This has nothing to do, per se, with the fact that you consulted "AI" (more aptly and less sensationally called a "correlation engine") to prepare this work. As I explained before, it is your professed lack of understanding that should have prevented you from offering this submission in the first place or, I would have hoped, caused the project owners to reject the submission in reinforcement of the general principles of any intellectual endeavor. My concept of ethics in this arena is deeply influenced by my engagement with open-source software projects, a hobby which can steal time from other personal and professional activities but offers in exchange opportunities for meaningful interactions and personal growth.

Despite my objection, I devoted time to understand your contribution and offer some guidance. This provided me an opportunity to learn a little about the internal workings of ZFS, and I hoped it would be a learning opportunity for you as well.

Your last response was very obviously produced by the same software responsible for the initial submission. After a few introductory sentences written in an informal, human voice consistent with other messages in this thread, the body of the message shifts to a structured, formal style that is characteristic of every interaction I've ever had with a large language model. Rather than developing a deep understanding of your contribution and defending it in your own terms, you seem to continue merely as a conduit to ChatGPT.

I am unwilling to devote further time to interacting with a correlation engine by proxy.

[QORTEC]

I am unwilling to devote further time

Thank you for all the time and help you devoted to this pull request.

human voice consistent with other messages in this thread

If I understand correctly, the messages you are referring to where written using the GitHub Mobile app. Those replies where written while I was on my way to and from work, also I generally don't like to using AI for responses/conversations with others.

formal style that is characteristic of every interaction I've ever had with a large language model.

I've had a couple of days off, and was able to dedicate more time to working on the PR.
The reason for behind the tone shift is multi-fold;

• I am engaging using a different medium
• my posts where of a different nature
  • for status updates and thoughts on technical issues, I prefer using formal language

My recent posts/messages have been written with AI Assistance as defined by RoyalRoad AI Text Policy.

• I ask the AI to "review my text for spelling and grammar errors"
  • unless I specifically state that "I am happy with the wording"
    the AI will still continue to suggest alternative wording along with the requested fixes
• I also use AI to help format/express ideas that I had difficulty writing down
  • by providing sentence fragments that are poorly connected the AI can often find a path forwards in express these thoughts in a coherent manner

[QORTEC]

The main issues I've found are comments and log messages that need fixing/updating.
Next I will recommit/rewrite my Git history so the code additions/changes are easier to follow.
Functionally speaking the PR looks good, so I could probably remove the Draft status when these changes are committed.

[QORTEC]

This method of validating filesystem feels a bit messy. While looking through the ZFS codebase, I haven't come across a better (acceptable) way to accomplish this.

If we only needed to validate filesystem, then we could use zfs_dataset_exists(). However, this is not acceptable for the CLI since the function only returns a simple true or false, without providing any error messages.

The best alternative I've thought of so far is to modify zfs_dataset_exists() by adding the seven lines of code necessary to enable error reporting capabilities in the function, matching those of zfs_open().

To maintain compatibility, we can call this function zfs_dataset_exists_verbose() and control the error reporting via the boolean_t error property. Next, we can make zfs_dataset_exists() a helper function that executes zfs_dataset_exists_verbose() with error reporting disabled.

Click here to see an example of the proposed code:

/*
 * Finds whether the dataset of the given type(s) exists.
 */
boolean_t
zfs_dataset_exists_verbose(libzfs_handle_t *hdl, const char *path, zfs_type_t types, boolean_t error)
{
	zfs_handle_t *zhp;
	char errbuf[ERRBUFLEN];

	(void) snprintf(errbuf, sizeof (errbuf),
	    dgettext(TEXT_DOMAIN, "cannot open '%s'"), path);

	/*
	 * Validate the name before we even try to open it.
	 */
	if (!zfs_validate_name(hdl, path, types, B_FALSE)) {
		if (error)
			(void) zfs_error(hdl, EZFS_INVALIDNAME, errbuf);
		return (B_FALSE);
	}

	/*
	 * Try to get stats for the dataset, which will tell us if it exists.
	 */
	if ((zhp = make_dataset_handle(hdl, path)) != NULL) {
		int ds_type = zhp->zfs_type;

		zfs_close(zhp);
		if (types & ds_type)
			return (B_TRUE);
	}

	if (error)
		(void) zfs_standard_error(hdl, errno, errbuf);
	return (B_FALSE);
}

boolean_t
zfs_dataset_exists(libzfs_handle_t *hdl, const char *path, zfs_type_t types)
{
	return (zfs_dataset_exists_verbose(hdl, path, types, B_TRUE));
}

libzfs_dataset.c

_LIBZFS_H boolean_t zfs_dataset_exists_verbose(libzfs_handle_t *, const char *,
    zfs_type_t, boolean_t);

include/libzfs.h

/* share_mount(): Validate filesystem */
if (filesystem != NULL && !zfs_dataset_exists_verbose(g_zfs,
    filesystem, ZFS_TYPE_FILESYSTEM, B_TRUE)) {
	free(options);
	return (1);
}

/* unshare_unmount() Validate filesystem */
if (filesystem != NULL && !zfs_dataset_exists_verbose(g_zfs,
    filesystem, ZFS_TYPE_FILESYSTEM, B_TRUE))
	return (1);

As seen above, the validation code looks cleaner and is easier to understand. However, I'm unsure about the general usefulness of the proposed zfs_dataset_exists_verbose() function and whether this should be added to the PR.

[amotin]

This is already fixed by #16015.