From 1a8f4e61c66ab9c4912cf956d69e281decb2eb74 Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Mon, 2 Dec 2024 07:34:18 -0600 Subject: [PATCH 1/3] Convert listdir and mkdir to api_method This commit converts the two filesystem API endpoints `filesystem.listdir` and `filesystem.mkdir` to new api_method. As part of this change, the deprecated way of calling filesystem.mkdir using positional arguments was removed. --- .../middlewared/api/v25_04_0/filesystem.py | 101 ++++++++++++++++++ .../middlewared/plugins/filesystem.py | 73 ++++--------- .../middlewared/utils/filesystem/stat_x.py | 16 +-- tests/api2/test_190_filesystem.py | 4 +- tests/api2/test_435_smb_registry.py | 6 +- tests/api2/test_account_privilege_role.py | 2 +- tests/api2/test_large_message.py | 2 +- 7 files changed, 136 insertions(+), 68 deletions(-) diff --git a/src/middlewared/middlewared/api/v25_04_0/filesystem.py b/src/middlewared/middlewared/api/v25_04_0/filesystem.py index f7d03997dd35..bbe7ce803490 100644 --- a/src/middlewared/middlewared/api/v25_04_0/filesystem.py +++ b/src/middlewared/middlewared/api/v25_04_0/filesystem.py @@ -3,17 +3,24 @@ NonEmptyString, UnixPerm, single_argument_args, + query_result ) from pydantic import Field, model_validator from typing import Literal, Self from middlewared.utils.filesystem.acl import ( ACL_UNDEFINED_ID, ) +from middlewared.utils.filesystem.stat_x import ( + StatxEtype, +) from .acl import AceWhoId +from .common import QueryFilters, QueryOptions __all__ = [ 'FilesystemChownArgs', 'FilesystemChownResult', 'FilesystemSetPermArgs', 'FilesystemSetPermResult', + 'FilesystemListdirArgs', 'FilesystemListdirResult', + 'FilesystemMkdirArgs', 'FilesystemMkdirResult', ] @@ -79,3 +86,97 @@ def payload_is_actionable(self) -> Self: class FilesystemSetPermResult(BaseModel): result: Literal[None] + + +FILESYSTEM_STATX_ATTRS = Literal[ + 'COMPRESSED', + 'APPEND', + 'NODUMP', + 'IMMUTABLE', + 'AUTOMOUNT', + 'MOUNT_ROOT', + 'VERIFY', + 'DAX' +] + + +FILESYSTEM_ZFS_ATTRS = Literal[ + 'READONLY', + 'HIDDEN', + 'SYSTEM', + 'ARCHIVE', + 'IMMUTABLE', + 'NOUNLINK', + 'APPENDONLY', + 'NODUMP', + 'OPAQUE', + 'AV_QUARANTINED', + 'AV_MODIFIED', + 'REPARSE', + 'OFFLINE', + 'SPARSE' +] + + +class FilesystemDirEntry(BaseModel): + name: NonEmptyString + """ Entry's base name. """ + path: NonEmptyString + """ Entry's full path. """ + realpath: NonEmptyString + """ Canonical path of the entry, eliminating any symbolic links""" + type: Literal[ + StatxEtype.DIRECTORY, + StatxEtype.FILE, + StatxEtype.SYMLINK, + StatxEtype.OTHER, + ] + size: int + """ Size in bytes of a plain file. """ + allocation_size: int + mode: int + """ Entry's mode including file type information and file permission bits. """ + mount_id: int + """ The mount ID of the mount containing the entry. This corresponds to the number in first + field of /proc/self/mountinfo. """ + acl: bool + """ Specifies whether ACL is present on the entry. If this is the case then file permission + bits as reported in `mode` may not be representative of the actual permissions. """ + uid: int + """ User ID of the entry's owner. """ + gid: int + """ Group ID of the entry's owner. """ + is_mountpoint: bool + """ Specifies whether the entry is also the mountpoint of a filesystem. """ + is_ctldir: bool + """ Specifies whether the entry is located within the ZFS ctldir (for example a snapshot). """ + attributes: list[FILESYSTEM_STATX_ATTRS] + """ Extra file attribute indicators for entry as returned by statx. """ + xattrs: list[NonEmptyString] + """ List of xattr names of extended attributes on file. """ + zfs_attrs: list[FILESYSTEM_ZFS_ATTRS] | None + """ List of extra ZFS-related file attribute indicators on file. Will be None type if filesystem is not ZFS. """ + + +class FilesystemListdirArgs(BaseModel): + path: NonEmptyString + query_filters: QueryFilters = [] + query_options: QueryOptions = QueryOptions() + + +FilesystemListdirResult = query_result(FilesystemDirEntry) + + +class FilesystemMkdirOptions(BaseModel): + mode: UnixPerm = '755' + raise_chmod_error: bool = True + + +@single_argument_args('filesystem_mkdir') +class FilesystemMkdirArgs(BaseModel): + path: NonEmptyString + options: FilesystemMkdirOptions = Field(default=FilesystemMkdirOptions()) + + +class FilesystemMkdirResult(BaseModel): + result: FilesystemDirEntry diff --git a/src/middlewared/middlewared/plugins/filesystem.py b/src/middlewared/middlewared/plugins/filesystem.py index 6bae98c57438..7a7a50d14c64 100644 --- a/src/middlewared/middlewared/plugins/filesystem.py +++ b/src/middlewared/middlewared/plugins/filesystem.py @@ -10,11 +10,16 @@ import pyinotify from itertools import product +from middlewared.api import api_method +from middlewared.api.current import ( + FilesystemListdirArgs, FilesystemListdirResult, + FilesystemMkdirArgs, FilesystemMkdirResult, +) from middlewared.event import EventSource from middlewared.plugins.pwenc import PWENC_FILE_SECRET, PWENC_FILE_SECRET_MODE from middlewared.plugins.docker.state_utils import IX_APPS_DIR_NAME -from middlewared.schema import accepts, Bool, Dict, Float, Int, List, Ref, returns, Path, Str, UnixPerm -from middlewared.service import private, CallError, filterable_returns, filterable, Service, job +from middlewared.schema import accepts, Bool, Dict, Float, Int, List, Ref, returns, Path, Str +from middlewared.service import private, CallError, filterable, Service, job from middlewared.utils import filter_list from middlewared.utils.filesystem import attrs, stat_x from middlewared.utils.filesystem.acl import acl_is_present @@ -116,21 +121,7 @@ def mount_info(self, filters, options): mntinfo = getmntinfo() return filter_list(list(mntinfo.values()), filters, options) - @accepts(Dict( - 'filesystem_mkdir', - Str('path'), - Dict( - 'options', - UnixPerm('mode', default='755'), - Bool('raise_chmod_error', default=True) - ), - ), deprecated=[( - lambda args: len(args) == 1 and isinstance(args[0], str), - lambda mkdir_path: [{ - 'path': mkdir_path - }] - )], roles=['FILESYSTEM_DATA_WRITE']) - @returns(Ref('path_entry')) + @api_method(FilesystemMkdirArgs, FilesystemMkdirResult, roles=['FILESYSTEM_DATA_WRITE']) def mkdir(self, data): """ Create a directory at the specified path. @@ -162,8 +153,10 @@ def mkdir(self, data): raise CallError(f'{path}: path not permitted', errno.EPERM) os.mkdir(path, mode=mode) - stat = p.stat() - if statlib.S_IMODE(stat.st_mode) != mode: + st = stat_x.statx_entry_impl(p, None) + stat = st['st'] + + if statlib.S_IMODE(stat.stx_mode) != mode: # This may happen if requested mode is greater than umask # or if underlying dataset has restricted aclmode and ACL is present try: @@ -183,13 +176,16 @@ def mkdir(self, data): 'path': path, 'realpath': realpath, 'type': 'DIRECTORY', - 'size': stat.st_size, - 'mode': stat.st_mode, + 'size': stat.stx_size, + 'allocation_size': stat.stx_blocks * 512, + 'mode': stat.stx_mode, 'acl': acl_is_present(os.listxattr(path)), - 'uid': stat.st_uid, - 'gid': stat.st_gid, + 'uid': stat.stx_uid, + 'gid': stat.stx_gid, 'is_mountpoint': False, 'is_ctldir': False, + 'mount_id': st['st'].stx_mnt_id, + 'attributes': st['attributes'], 'xattrs': [], 'zfs_attrs': ['ARCHIVE'] } @@ -221,36 +217,7 @@ def listdir_request_mask(self, select): return request_mask - @accepts( - Str('path', required=True), - Ref('query-filters'), - Ref('query-options'), - roles=['FILESYSTEM_ATTRS_READ'] - ) - @filterable_returns(Dict( - 'path_entry', - Str('name', required=True), - Path('path', required=True), - Path('realpath', required=True), - Str('type', required=True, enum=['DIRECTORY', 'FILE', 'SYMLINK', 'OTHER']), - Int('size', required=True, null=True), - Int('allocation_size', required=True, null=True), - Int('mode', required=True, null=True), - Int('mount_id', required=True, null=True), - Bool('acl', required=True, null=True), - Int('uid', required=True, null=True), - Int('gid', required=True, null=True), - Bool('is_mountpoint', required=True), - Bool('is_ctldir', required=True), - List( - 'attributes', - required=True, - items=[Str('statx_attribute', enum=[attr.name for attr in stat_x.StatxAttr])] - ), - List('xattrs', required=True, null=True), - List('zfs_attrs', required=True, null=True), - register=True - )) + @api_method(FilesystemListdirArgs, FilesystemListdirResult, roles=['FILESYSTEM_ATTRS_READ']) def listdir(self, path, filters, options): """ Get the contents of a directory. diff --git a/src/middlewared/middlewared/utils/filesystem/stat_x.py b/src/middlewared/middlewared/utils/filesystem/stat_x.py index abb2c27da1fd..e87798d8b4c4 100644 --- a/src/middlewared/middlewared/utils/filesystem/stat_x.py +++ b/src/middlewared/middlewared/utils/filesystem/stat_x.py @@ -8,16 +8,16 @@ import os import ctypes import stat as statlib -from enum import auto, Enum, IntFlag +from enum import IntFlag, StrEnum from .constants import AT_FDCWD from .utils import path_in_ctldir -class StatxEtype(Enum): - DIRECTORY = auto() - FILE = auto() - SYMLINK = auto() - OTHER = auto() +class StatxEtype(StrEnum): + DIRECTORY = 'DIRECTORY' + FILE = 'FILE' + SYMLINK = 'SYMLINK' + OTHER = 'OTHER' class ATFlags(IntFlag): @@ -177,8 +177,8 @@ def statx_entry_impl(entry, dir_fd=None, get_ctldir=True): # This is equivalent to lstat() call out['st'] = statx( path, - dir_fd = dir_fd, - flags = __statx_lstat_flags + dir_fd=dir_fd, + flags=__statx_lstat_flags ) except FileNotFoundError: return None diff --git a/tests/api2/test_190_filesystem.py b/tests/api2/test_190_filesystem.py index fd23fd648ba8..c3f991de8286 100644 --- a/tests/api2/test_190_filesystem.py +++ b/tests/api2/test_190_filesystem.py @@ -75,9 +75,9 @@ def test_immutable_flag(): # 2) "is_immutable_set" returns sane response if flag_set: with pytest.raises(PermissionError): - call("filesystem.mkdir", f"{t_child_path}_{flag_set}") + call('filesystem.mkdir', {'path': f"{t_child_path}_{flag_set}"}) else: - call("filesystem.mkdir", f"{t_child_path}_{flag_set}") + call('filesystem.mkdir', {'path': f"{t_child_path}_{flag_set}"}) is_immutable = 'IMMUTABLE' in call('filesystem.stat', t_path)['attributes'] err = "Immutable flag is still not set" diff --git a/tests/api2/test_435_smb_registry.py b/tests/api2/test_435_smb_registry.py index bfe6db31184b..a048dfe86746 100644 --- a/tests/api2/test_435_smb_registry.py +++ b/tests/api2/test_435_smb_registry.py @@ -70,7 +70,7 @@ def create_smb_share(path, share_name, mkdir=False, options=None): cr_opts = options or {} if mkdir: - call('filesystem.mkdir', path) + call('filesystem.mkdir', {'path': path, 'options': {'raise_chmod_error': False}}) with smb_share(path, share_name, cr_opts) as share: yield share @@ -82,7 +82,7 @@ def setup_smb_shares(mountpoint): for share in SHARES: share_path = os.path.join(mountpoint, share) - call('filesystem.mkdir', share_path) + call('filesystem.mkdir', {'path': share_path, 'options': {'raise_chmod_error': False}}) new_share = call('sharing.smb.create', { 'comment': 'My Test SMB Share', 'name': share, @@ -342,7 +342,7 @@ def test__delete_shares(setup_for_tests): def test__create_homes_share(setup_for_tests): mp, ds, share_dict = setup_for_tests home_path = os.path.join(mp, 'HOME_SHARE') - call('filesystem.mkdir', home_path) + call('filesystem.mkdir', {'path': home_path, 'options': {'raise_chmod_error': False}}) new_share = call('sharing.smb.create', { "comment": "My Test SMB Share", diff --git a/tests/api2/test_account_privilege_role.py b/tests/api2/test_account_privilege_role.py index 88d572b74ce2..c1284a6065bb 100644 --- a/tests/api2/test_account_privilege_role.py +++ b/tests/api2/test_account_privilege_role.py @@ -113,7 +113,7 @@ def test_readonly_can_not_call_method(): with pytest.raises(CallError) as ve: # fails with EPERM if API access granted - c.call("filesystem.mkdir", "/foo") + c.call("filesystem.mkdir", {"path": "/foo"}) assert ve.value.errno == errno.EACCES diff --git a/tests/api2/test_large_message.py b/tests/api2/test_large_message.py index e97efb443248..a0b318d847fe 100644 --- a/tests/api2/test_large_message.py +++ b/tests/api2/test_large_message.py @@ -13,7 +13,7 @@ def test_large_message_default(): with pytest.raises(ClientException) as ce: with client() as c: - c.call('filesystem.mkdir', LARGE_PAYLOAD_1) + c.call('filesystem.mkdir', {'path': LARGE_PAYLOAD_1}) assert MSG_TOO_BIG_ERR in ce.value.error From 9f408a45d2f916bd638725c59047811ec4b3098b Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Mon, 2 Dec 2024 11:50:02 -0600 Subject: [PATCH 2/3] Expand docstring --- .../middlewared/api/v25_04_0/filesystem.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/middlewared/middlewared/api/v25_04_0/filesystem.py b/src/middlewared/middlewared/api/v25_04_0/filesystem.py index bbe7ce803490..d9310d3d4502 100644 --- a/src/middlewared/middlewared/api/v25_04_0/filesystem.py +++ b/src/middlewared/middlewared/api/v25_04_0/filesystem.py @@ -132,26 +132,27 @@ class FilesystemDirEntry(BaseModel): StatxEtype.OTHER, ] size: int - """ Size in bytes of a plain file. """ + """ Size in bytes of a plain file. This corresonds with stx_size. """ allocation_size: int + """ Allocated size of file. Calculated by multiplying stx_blocks by 512. """ mode: int - """ Entry's mode including file type information and file permission bits. """ + """ Entry's mode including file type information and file permission bits. This corresponds with stx_mode. """ mount_id: int """ The mount ID of the mount containing the entry. This corresponds to the number in first - field of /proc/self/mountinfo. """ + field of /proc/self/mountinfo and stx_mnt_id. """ acl: bool """ Specifies whether ACL is present on the entry. If this is the case then file permission bits as reported in `mode` may not be representative of the actual permissions. """ uid: int - """ User ID of the entry's owner. """ + """ User ID of the entry's owner. This corresponds with stx_uid. """ gid: int - """ Group ID of the entry's owner. """ + """ Group ID of the entry's owner. This corresponds with stx_gid. """ is_mountpoint: bool """ Specifies whether the entry is also the mountpoint of a filesystem. """ is_ctldir: bool """ Specifies whether the entry is located within the ZFS ctldir (for example a snapshot). """ attributes: list[FILESYSTEM_STATX_ATTRS] - """ Extra file attribute indicators for entry as returned by statx. """ + """ Extra file attribute indicators for entry as returned by statx. Expanded from stx_attributes. """ xattrs: list[NonEmptyString] """ List of xattr names of extended attributes on file. """ zfs_attrs: list[FILESYSTEM_ZFS_ATTRS] | None From 980f3fe66f9bcba3823edb7615e5b41ac8a19711 Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Mon, 2 Dec 2024 14:09:49 -0600 Subject: [PATCH 3/3] Flake8 fix --- tests/api2/test_435_smb_registry.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/api2/test_435_smb_registry.py b/tests/api2/test_435_smb_registry.py index a048dfe86746..87af52dd31e3 100644 --- a/tests/api2/test_435_smb_registry.py +++ b/tests/api2/test_435_smb_registry.py @@ -70,7 +70,7 @@ def create_smb_share(path, share_name, mkdir=False, options=None): cr_opts = options or {} if mkdir: - call('filesystem.mkdir', {'path': path, 'options': {'raise_chmod_error': False}}) + call('filesystem.mkdir', {'path': path, 'options': {'raise_chmod_error': False}}) with smb_share(path, share_name, cr_opts) as share: yield share @@ -82,7 +82,7 @@ def setup_smb_shares(mountpoint): for share in SHARES: share_path = os.path.join(mountpoint, share) - call('filesystem.mkdir', {'path': share_path, 'options': {'raise_chmod_error': False}}) + call('filesystem.mkdir', {'path': share_path, 'options': {'raise_chmod_error': False}}) new_share = call('sharing.smb.create', { 'comment': 'My Test SMB Share', 'name': share, @@ -342,7 +342,7 @@ def test__delete_shares(setup_for_tests): def test__create_homes_share(setup_for_tests): mp, ds, share_dict = setup_for_tests home_path = os.path.join(mp, 'HOME_SHARE') - call('filesystem.mkdir', {'path': home_path, 'options': {'raise_chmod_error': False}}) + call('filesystem.mkdir', {'path': home_path, 'options': {'raise_chmod_error': False}}) new_share = call('sharing.smb.create', { "comment": "My Test SMB Share",