From 4ccada4ab754fbf2e2beb7adf24fdf4ab3f25952 Mon Sep 17 00:00:00 2001 From: Shachar Sharon Date: Thu, 23 May 2024 17:15:40 +0300 Subject: [PATCH 1/3] vfs_ceph: replace WRAP_RETURN macro with convenience helpers The WRAP_RETURN is a non-hygienic macro, and as such has the potential of creating bogus code (e.g. 'return WRAP_RETURN(ret);' which existed in the code in the past but did not yield any compiler warning). Prefer simple convenience helper functions instead, which are also type safe. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15686 Signed-off-by: Shachar Sharon Reviewed-by: Andrew Bartlett Reviewed-by: David Disseldorp Reviewed-by: Ralph Boehme Reviewed-by: Guenther Deschner Reviewed-by: Anoop C S (cherry picked from commit 691a397b2707f2924e3f6910c9c574e01d811a97) --- source3/modules/vfs_ceph.c | 109 ++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 51 deletions(-) diff --git a/source3/modules/vfs_ceph.c b/source3/modules/vfs_ceph.c index c9ee5414f03..a180e96cea5 100644 --- a/source3/modules/vfs_ceph.c +++ b/source3/modules/vfs_ceph.c @@ -54,16 +54,30 @@ #define llu(_var) ((long long unsigned)_var) /* - * Note, libcephfs's return code model is to return -errno! So we have to - * convert to what Samba expects, which is to set errno to -return and return -1 + * Note, libcephfs's return code model is to return -errno. Thus we have to + * convert to what Samba expects: set errno to non-negative value and return -1. + * + * Using convenience helper functions to avoid non-hygienic macro. */ -#define WRAP_RETURN(_res) \ - errno = 0; \ - if (_res < 0) { \ - errno = -_res; \ - return -1; \ - } \ - return _res \ +static inline int status_code(int ret) +{ + if (ret < 0) { + errno = -ret; + return -1; + } + errno = 0; + return ret; +} + +static inline ssize_t lstatus_code(intmax_t ret) +{ + if (ret < 0) { + errno = -((int)ret); + return -1; + } + errno = 0; + return (ssize_t)ret; +} /* * Track unique connections, as virtual mounts, to cephfs file systems. @@ -344,7 +358,7 @@ static uint64_t cephwrap_disk_free(struct vfs_handle_struct *handle, return *dfree; } else { DBG_DEBUG("[CEPH] ceph_statfs returned %d\n", ret); - WRAP_RETURN(ret); + return status_code(ret); } } @@ -401,7 +415,7 @@ static int cephwrap_statvfs(struct vfs_handle_struct *handle, ret = ceph_statfs(handle->data, smb_fname->base_name, &statvfs_buf); if (ret < 0) { - WRAP_RETURN(ret); + return status_code(ret); } statbuf->OptimalTransferSize = statvfs_buf.f_frsize; @@ -493,7 +507,7 @@ static int cephwrap_mkdirat(struct vfs_handle_struct *handle, DBG_DEBUG("[CEPH] mkdirat(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #else struct smb_filename *full_fname = NULL; @@ -511,7 +525,7 @@ static int cephwrap_mkdirat(struct vfs_handle_struct *handle, TALLOC_FREE(full_fname); - WRAP_RETURN(result); + return status_code(result); #endif } @@ -522,7 +536,7 @@ static int cephwrap_closedir(struct vfs_handle_struct *handle, DIR *dirp) DBG_DEBUG("[CEPH] closedir(%p, %p)\n", handle, dirp); result = ceph_closedir(handle->data, (struct ceph_dir_result *) dirp); DBG_DEBUG("[CEPH] closedir(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } /* File operations */ @@ -604,7 +618,7 @@ out: TALLOC_FREE(name); fsp->fsp_flags.have_proc_fds = false; DBG_DEBUG("[CEPH] open(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_close(struct vfs_handle_struct *handle, files_struct *fsp) @@ -614,8 +628,7 @@ static int cephwrap_close(struct vfs_handle_struct *handle, files_struct *fsp) DBG_DEBUG("[CEPH] close(%p, %p)\n", handle, fsp); result = ceph_close(handle->data, fsp_get_pathref_fd(fsp)); DBG_DEBUG("[CEPH] close(...) = %d\n", result); - - WRAP_RETURN(result); + return status_code(result); } static ssize_t cephwrap_pread(struct vfs_handle_struct *handle, files_struct *fsp, void *data, @@ -627,7 +640,7 @@ static ssize_t cephwrap_pread(struct vfs_handle_struct *handle, files_struct *fs result = ceph_read(handle->data, fsp_get_io_fd(fsp), data, n, offset); DBG_DEBUG("[CEPH] pread(...) = %llu\n", llu(result)); - WRAP_RETURN(result); + return lstatus_code(result); } struct cephwrap_pread_state { @@ -690,7 +703,7 @@ static ssize_t cephwrap_pwrite(struct vfs_handle_struct *handle, files_struct *f DBG_DEBUG("[CEPH] pwrite(%p, %p, %p, %llu, %llu)\n", handle, fsp, data, llu(n), llu(offset)); result = ceph_write(handle->data, fsp_get_io_fd(fsp), data, n, offset); DBG_DEBUG("[CEPH] pwrite(...) = %llu\n", llu(result)); - WRAP_RETURN(result); + return lstatus_code(result); } struct cephwrap_pwrite_state { @@ -751,7 +764,7 @@ static off_t cephwrap_lseek(struct vfs_handle_struct *handle, files_struct *fsp, DBG_DEBUG("[CEPH] cephwrap_lseek\n"); result = ceph_lseek(handle->data, fsp_get_io_fd(fsp), offset, whence); - WRAP_RETURN(result); + return lstatus_code(result); } static ssize_t cephwrap_sendfile(struct vfs_handle_struct *handle, int tofd, files_struct *fromfsp, const DATA_BLOB *hdr, @@ -818,7 +831,7 @@ static int cephwrap_renameat(struct vfs_handle_struct *handle, TALLOC_FREE(full_fname_src); TALLOC_FREE(full_fname_dst); - WRAP_RETURN(result); + return status_code(result); } /* @@ -924,7 +937,7 @@ static int cephwrap_stat(struct vfs_handle_struct *handle, SAMBA_STATX_ATTR_MASK, 0); DBG_DEBUG("[CEPH] statx(...) = %d\n", result); if (result < 0) { - WRAP_RETURN(result); + return status_code(result); } init_stat_ex_from_ceph_statx(&smb_fname->st, &stx); @@ -943,7 +956,7 @@ static int cephwrap_fstat(struct vfs_handle_struct *handle, files_struct *fsp, S SAMBA_STATX_ATTR_MASK, 0); DBG_DEBUG("[CEPH] fstat(...) = %d\n", result); if (result < 0) { - WRAP_RETURN(result); + return status_code(result); } init_stat_ex_from_ceph_statx(sbuf, &stx); @@ -987,7 +1000,7 @@ static int cephwrap_fstatat(struct vfs_handle_struct *handle, DBG_DEBUG("[CEPH] fstatat(...) = %d\n", result); if (result < 0) { - WRAP_RETURN(result); + return status_code(result); } init_stat_ex_from_ceph_statx(sbuf, &stx); @@ -1013,7 +1026,7 @@ static int cephwrap_lstat(struct vfs_handle_struct *handle, SAMBA_STATX_ATTR_MASK, AT_SYMLINK_NOFOLLOW); DBG_DEBUG("[CEPH] lstat(...) = %d\n", result); if (result < 0) { - WRAP_RETURN(result); + return status_code(result); } init_stat_ex_from_ceph_statx(&smb_fname->st, &stx); @@ -1095,7 +1108,7 @@ static int cephwrap_unlinkat(struct vfs_handle_struct *handle, smb_fname->base_name, flags); DBG_DEBUG("[CEPH] unlinkat(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #else struct smb_filename *full_fname = NULL; @@ -1122,7 +1135,7 @@ static int cephwrap_unlinkat(struct vfs_handle_struct *handle, } TALLOC_FREE(full_fname); DBG_DEBUG("[CEPH] unlink(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #endif } @@ -1145,7 +1158,7 @@ static int cephwrap_fchmod(struct vfs_handle_struct *handle, files_struct *fsp, mode); } DBG_DEBUG("[CEPH] fchmod(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_fchown(struct vfs_handle_struct *handle, files_struct *fsp, uid_t uid, gid_t gid) @@ -1172,7 +1185,7 @@ static int cephwrap_fchown(struct vfs_handle_struct *handle, files_struct *fsp, } DBG_DEBUG("[CEPH] fchown(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_lchown(struct vfs_handle_struct *handle, @@ -1184,7 +1197,7 @@ static int cephwrap_lchown(struct vfs_handle_struct *handle, DBG_DEBUG("[CEPH] lchown(%p, %s, %d, %d)\n", handle, smb_fname->base_name, uid, gid); result = ceph_lchown(handle->data, smb_fname->base_name, uid, gid); DBG_DEBUG("[CEPH] lchown(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_chdir(struct vfs_handle_struct *handle, @@ -1194,7 +1207,7 @@ static int cephwrap_chdir(struct vfs_handle_struct *handle, DBG_DEBUG("[CEPH] chdir(%p, %s)\n", handle, smb_fname->base_name); result = ceph_chdir(handle->data, smb_fname->base_name); DBG_DEBUG("[CEPH] chdir(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } static struct smb_filename *cephwrap_getwd(struct vfs_handle_struct *handle, @@ -1234,13 +1247,13 @@ static int strict_allocate_ftruncate(struct vfs_handle_struct *handle, files_str /* Shrink - just ftruncate. */ if (pst->st_ex_size > len) { result = ceph_ftruncate(handle->data, fsp_get_io_fd(fsp), len); - WRAP_RETURN(result); + return status_code(result); } space_to_write = len - pst->st_ex_size; result = ceph_fallocate(handle->data, fsp_get_io_fd(fsp), 0, pst->st_ex_size, space_to_write); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_ftruncate(struct vfs_handle_struct *handle, files_struct *fsp, off_t len) @@ -1254,7 +1267,7 @@ static int cephwrap_ftruncate(struct vfs_handle_struct *handle, files_struct *fs } result = ceph_ftruncate(handle->data, fsp_get_io_fd(fsp), len); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_fallocate(struct vfs_handle_struct *handle, @@ -1270,7 +1283,7 @@ static int cephwrap_fallocate(struct vfs_handle_struct *handle, /* unsupported mode flags are rejected by libcephfs */ result = ceph_fallocate(handle->data, fsp_get_io_fd(fsp), mode, offset, len); DBG_DEBUG("[CEPH] fallocate(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); } static bool cephwrap_lock(struct vfs_handle_struct *handle, files_struct *fsp, int op, off_t offset, off_t count, int type) @@ -1362,7 +1375,7 @@ static int cephwrap_symlinkat(struct vfs_handle_struct *handle, dirfd, new_smb_fname->base_name); DBG_DEBUG("[CEPH] symlinkat(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #else struct smb_filename *full_fname = NULL; @@ -1382,7 +1395,7 @@ static int cephwrap_symlinkat(struct vfs_handle_struct *handle, full_fname->base_name); TALLOC_FREE(full_fname); DBG_DEBUG("[CEPH] symlink(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #endif } @@ -1410,7 +1423,7 @@ static int cephwrap_readlinkat(struct vfs_handle_struct *handle, bufsiz); DBG_DEBUG("[CEPH] readlinkat(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #else struct smb_filename *full_fname = NULL; @@ -1427,7 +1440,7 @@ static int cephwrap_readlinkat(struct vfs_handle_struct *handle, result = ceph_readlink(handle->data, full_fname->base_name, buf, bufsiz); TALLOC_FREE(full_fname); DBG_DEBUG("[CEPH] readlink(...) = %d\n", result); - WRAP_RETURN(result); + return status_code(result); #endif } @@ -1466,7 +1479,7 @@ static int cephwrap_linkat(struct vfs_handle_struct *handle, DBG_DEBUG("[CEPH] link(...) = %d\n", result); TALLOC_FREE(full_fname_old); TALLOC_FREE(full_fname_new); - WRAP_RETURN(result); + return status_code(result); } static int cephwrap_mknodat(struct vfs_handle_struct *handle, @@ -1491,7 +1504,7 @@ static int cephwrap_mknodat(struct vfs_handle_struct *handle, TALLOC_FREE(full_fname); - WRAP_RETURN(result); + return status_code(result); } /* @@ -1600,10 +1613,7 @@ static ssize_t cephwrap_fgetxattr(struct vfs_handle_struct *handle, size); } DBG_DEBUG("[CEPH] fgetxattr(...) = %d\n", ret); - if (ret < 0) { - WRAP_RETURN(ret); - } - return (ssize_t)ret; + return lstatus_code(ret); } static ssize_t cephwrap_flistxattr(struct vfs_handle_struct *handle, struct files_struct *fsp, char *list, size_t size) @@ -1629,10 +1639,7 @@ static ssize_t cephwrap_flistxattr(struct vfs_handle_struct *handle, struct file size); } DBG_DEBUG("[CEPH] flistxattr(...) = %d\n", ret); - if (ret < 0) { - WRAP_RETURN(ret); - } - return (ssize_t)ret; + return lstatus_code(ret); } static int cephwrap_fremovexattr(struct vfs_handle_struct *handle, struct files_struct *fsp, const char *name) @@ -1653,7 +1660,7 @@ static int cephwrap_fremovexattr(struct vfs_handle_struct *handle, struct files_ name); } DBG_DEBUG("[CEPH] fremovexattr(...) = %d\n", ret); - WRAP_RETURN(ret); + return status_code(ret); } static int cephwrap_fsetxattr(struct vfs_handle_struct *handle, struct files_struct *fsp, const char *name, const void *value, size_t size, int flags) @@ -1682,7 +1689,7 @@ static int cephwrap_fsetxattr(struct vfs_handle_struct *handle, struct files_str flags); } DBG_DEBUG("[CEPH] fsetxattr(...) = %d\n", ret); - WRAP_RETURN(ret); + return status_code(ret); } static bool cephwrap_aio_force(struct vfs_handle_struct *handle, struct files_struct *fsp) -- 2.46.0 From 5ae4a6efa047f6d2db81774ffed6aa0ba786084c Mon Sep 17 00:00:00 2001 From: Shachar Sharon Date: Thu, 30 May 2024 11:02:37 +0300 Subject: [PATCH 2/3] vfs_ceph: explicit cast to uint64_t upon failure of ceph_statfs When a call to 'ceph_statfs' from with 'cephwrap_disk_free' returns non-zero status do an explicit cast to uint64_t for the negative (-1) value returned by 'status_code'. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15686 Signed-off-by: Shachar Sharon Reviewed-by: Andrew Bartlett Reviewed-by: David Disseldorp Reviewed-by: Ralph Boehme Reviewed-by: Guenther Deschner Reviewed-by: Anoop C S (cherry picked from commit a7d34ec597fe810090d28bfda636b7450ecb06e5) --- source3/modules/vfs_ceph.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/modules/vfs_ceph.c b/source3/modules/vfs_ceph.c index a180e96cea5..cbe98a24c85 100644 --- a/source3/modules/vfs_ceph.c +++ b/source3/modules/vfs_ceph.c @@ -358,7 +358,7 @@ static uint64_t cephwrap_disk_free(struct vfs_handle_struct *handle, return *dfree; } else { DBG_DEBUG("[CEPH] ceph_statfs returned %d\n", ret); - return status_code(ret); + return (uint64_t)status_code(ret); } } -- 2.46.0 From a725325b479fd85061032ff8b04506b873518045 Mon Sep 17 00:00:00 2001 From: Shachar Sharon Date: Tue, 30 Jul 2024 09:55:44 +0300 Subject: [PATCH 3/3] vfs_ceph{_new}: do not set errno upon successful call to libcephfs There is code in Samba that expects errno from a previous system call to be preserved through a subsequent system call. Thus, avoid setting "errno = 0" in status_code() and lstatus_code() upon successful return from libcephfs API call. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15686 Signed-off-by: Shachar Sharon Reviewed-by: Guenther Deschner Reviewed-by: Anoop C S (cherry picked from commit a7f4e2bd47c7f4728f3ac8d90af693156a69c557) --- source3/modules/vfs_ceph.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/source3/modules/vfs_ceph.c b/source3/modules/vfs_ceph.c index cbe98a24c85..c36a095d47e 100644 --- a/source3/modules/vfs_ceph.c +++ b/source3/modules/vfs_ceph.c @@ -65,7 +65,6 @@ static inline int status_code(int ret) errno = -ret; return -1; } - errno = 0; return ret; } @@ -75,7 +74,6 @@ static inline ssize_t lstatus_code(intmax_t ret) errno = -((int)ret); return -1; } - errno = 0; return (ssize_t)ret; } -- 2.46.0