move set_mountpoint to ZFS, use new permission helper handling for create_fileset

src/simplezfs/exceptions.py

@@ -61,7 +61,8 @@ class ExternalPEHelperException(PEHelperException):
     '''
     Indicates a problem when running the external helper script.
     '''
-    def __init__(self, message: str, returncode: Optional[int], stdout: Optional[str] = None, stderr: Optional[str] = None) -> None:
+    def __init__(self, message: str, returncode: Optional[int], stdout: Optional[str] = None,
+                 stderr: Optional[str] = None) -> None:
         '''
         :param message: The message to carry.
         :param returncode: The programs return code.

src/simplezfs/pe_helper.py

@@ -7,7 +7,7 @@ import subprocess
 from typing import List, Optional
 
 from .exceptions import PEHelperException, ExternalPEHelperException
-from .validation import validate_dataset_path, validate_pool_name, validate_property_value
+from .validation import validate_dataset_path, validate_pool_name
 
 
 class PEHelperBase:

src/simplezfs/zfs.py

@@ -158,11 +158,10 @@ class ZFS:
         Sets or changes the mountpoint property of a fileset. While this can be achieved using the generic function
         :func:`~ZFS.set_property`, it allows for using the privilege escalation (PE) helper if so desired.
 
-        If the ``use_pe_helper`` *property* is set and the argument is None, an attempt is made to manipulate the
+        The argument ``use_pe_helper`` can overwrite the property of the same name. If the argument is None, the
-        property and the helper is used only if that fails. If the argument is ``True`` and no helper is set, a normal
+        properties value will be assumed. In any case, the function attempts to set the property on its own first. If
-        attempt is made as well but an error is returned if that does not work due to permissions. If the argument is
+        that fails, it evaluates if the PE helper should be used, and will error out if it should be used but has not
-        ``False``, a normal attempt is made and the helper not used even if the property is ``True`` and a helper is
+        been set. If the helper fails, a :class:`~simplezfs.exceptions.PEHelperException` is raised.
-        set, instead returning an error.
 
         :param fileset: The fileset to modify.
         :param mountpoint: The new value for the ``mountpoint`` property.
@@ -171,7 +170,24 @@ class ZFS:
         :raises DatasetNotFound: if the fileset could not be found.
         :raises ValidationError: if validating the parameters failed.
         '''
-        raise NotImplementedError(f'{self} has not implemented this function')
+        ds_type = self.get_property(fileset, 'type')
+        if ds_type != 'filesystem':
+            raise ValidationError(f'Dataset is not a filesystem and can\'t have its mountpoint set')
+
+        try:
+            self.set_property(fileset, 'mountpoint', mountpoint)
+        except PermissionError as e:
+            if self.pe_helper is not None:
+                real_use_pe_helper = use_pe_helper if use_pe_helper is not None else self.use_pe_helper
+
+                if real_use_pe_helper:
+                    log.info(f'Permission error when setting mountpoint for "{fileset}", retrying using PE helper')
+                    self.pe_helper.zfs_set_mountpoint(fileset, mountpoint)
+                else:
+                    log.error(f'Permission error when setting mountpoint for "{fileset}" and not using PE helper')
+                    raise e
+            else:
+                log.error(f'Permission error when setting mountpoint for "{fileset}" and PE helper is not set')
 
     def set_property(self, dataset: str, key: str, value: str, *, metadata: bool = False,
                      overwrite_metadata_namespace: Optional[str] = None) -> None:
@@ -377,6 +393,12 @@ class ZFS:
 
            This action requires the ``pe_helper`` on Linux when not running as `root`.
 
+        .. note::
+
+           If the function raises a ``PermissionError`` or ``PEHelperException``, the fileset may have been created,
+           but is missing its mountpoint property (along with it not being mounted) or other properties to which the
+           user has no permission to change (not in ``zfs allow``).
+
         :param name: Name of the new fileset (complete path in the ZFS hierarchy).
         :ppram mountpoint: Convenience parameter for setting/overwriting the moutpoint property
         :param properties: Dict of native properties to set.
@@ -416,6 +438,13 @@ class ZFS:
         Create a new volume of the given ``size`` (in bytes). If ``sparse`` is **True**, a sparse volume (also known
         as thin provisioned) will be created. If ``blocksize`` is given, it overwrites the ``blocksize`` property.
 
+        .. note::
+
+           Please read the note in :func:`~simplezfs.ZFS.create_filesystem` for permission handling for filesystems.
+           Generally, if the user does not have permission to set certain properties, the dataset may or may not have
+           been created but is missing the properties. It is up to the user of the library to clean up after
+           catching a ``PermissionError`.
+
         :param name: Name of the new volume (complete path in the ZFS hierarchy).
         :param size: The size (in `bytes`) for the new volume.
         :param sparse: Whether to create a sparse volume. Requires ``size`` to be set.

src/simplezfs/zfs_cli.py

@@ -120,28 +120,6 @@ class ZFSCli(ZFS):
             res.append(Dataset.from_string(name.strip()))
         return res
 
-    def set_mountpoint(self, fileset: str, mountpoint: str, *, use_pe_helper: Optional[bool] = False) -> None:
-        real_use_pe_helper = use_pe_helper if use_pe_helper is not None else self.use_pe_helper
-        ds_type = self.get_property(fileset, 'type')
-        if ds_type != 'filesystem':
-            raise ValidationError('Given fileset is not a filesystem, can\'t set mountpoint')
-
-        if not real_use_pe_helper:
-            self.set_property(fileset, 'mountpoint', mountpoint)
-        else:
-            if not self.pe_helper:
-                raise ValidationError('PE helper should be used, but is not defined')
-
-            args = [self.pe_helper, 'set_mountpoint', fileset, mountpoint]
-            log.debug(f'set_mountpoint: executing {args}')
-            proc = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding='utf-8')
-            log.debug(f'set_mountpoint returncode: {proc.returncode}')
-            log.debug(f'set_mountpoint stdout: {proc.stdout}')
-            log.debug(f'set_mountpoint stderr: {proc.stderr}')
-            # TODO log output
-            if proc.returncode != 0 or len(proc.stderr) > 0:
-                self.handle_command_error(proc, fileset)
-
     def handle_command_error(self, proc: subprocess.CompletedProcess, dataset: str = None) -> None:
         '''
         Handles errors that occured while running a command.
@@ -151,6 +129,7 @@ class ZFSCli(ZFS):
         :todo: propper exception!
         :raises DatasetNotFound: If zfs could not find the dataset it was requested to work with.
         :raises PropertyNotFound: If the could not find the property it was asked to work with.
+        :raises PermissionError: If zfs denied the operation, or if only root is allowed to carry it out.
         :raises Exception: tmp
         '''
         if 'dataset does not exist' in proc.stderr:
@@ -164,6 +143,8 @@ class ZFSCli(ZFS):
                 raise PropertyNotFound('invalid property')
         elif 'permission denied' in proc.stderr:
             raise PermissionError(proc.stderr)
+        elif 'filesystem successfully created, but it may only be mounted by root' in proc.stderr:
+            raise PermissionError(proc.stderr)
         raise Exception(f'Command execution "{" ".join(proc.args)}" failed: {proc.stderr}')
 
     def _set_property(self, dataset: str, key: str, value: str, is_metadata: bool) -> None:
@@ -278,32 +259,26 @@ class ZFSCli(ZFS):
                     if self.use_pe_helper:
                         # The mountpoint property may be set, in which case we can run the PE helper. If it is not
                         # set, we'd need to compute it based on the parent, but for now we simply error out.
-                        try:
+                        if properties and 'mountpoint' in properties:
                             mp = properties['mountpoint']
-                        except KeyError:
+                            if self.pe_helper is not None:
+                                log.info(f'Fileset {name} was created, using pe_helper to set the mountpoint')
+                                self.pe_helper.zfs_set_mountpoint(name, mp)
+                            else:
+                                msg = 'Fileset created partially but no PE helper set'
+                                log.error(msg)
+                                raise PermissionError(msg)
+                        else:
                             msg = 'Mountpoint property not set, can\'t run pe_helper'
                             log.error(msg)
                             raise PermissionError(msg)
 
-                        log.info(f'Fileset {name} was created, using pe_helper to set the mountpoint')
                     else:
                         log.info(f'Fileset {name} was created, but could not be mounted due to not running as root')
                         raise PermissionError(proc.stderr)
-
-            if self.use_pe_helper:
-                try:
-                    mp = properties['mountpoint']
-                except KeyError:
-                    raise ValidationError('Mountpoint not found in properties')
-                self._execute_pe_helper('create', recursive=recursive, mountpoint=properties['mountpoint'], name=name)
             else:
-                args = [self.__exe, 'create']
+                log.info('Filesystem created successfully')
-
+                return self.get_dataset_info(name)
-                if recursive:
-                    args += ['-p']
-
-                args += [name]
-                print(f'Executing {args}')
 
         elif dataset_type == DatasetType.VOLUME:
             assert size is not None