Address review feedback

include/seastar/core/file.hh

@@ -100,13 +100,6 @@ struct file_open_options {
     static constexpr uint32_t min_extent_size_hint_alignment{128u << 10}; // 128KB
 };
 
-struct group_details {
-    char *group_name;
-    char *group_password;
-    uint64_t group_gid;
-    char **group_member;
-};
-
 class file;
 class file_impl;
 class io_intent;

include/seastar/core/reactor.hh

@@ -21,6 +21,7 @@
 
 #pragma once
 
+#include <grp.h>
 #include <seastar/core/aligned_buffer.hh>
 #include <seastar/core/cacheline.hh>
 #include <seastar/core/circular_buffer.hh>
@@ -502,8 +503,8 @@ public:
     future<> touch_directory(std::string_view name, file_permissions permissions = file_permissions::default_dir_permissions) noexcept;
     future<std::optional<directory_entry_type>>  file_type(std::string_view name, follow_symlink = follow_symlink::yes) noexcept;
     future<stat_data> file_stat(std::string_view pathname, follow_symlink) noexcept;
-    future<> change_ownership_of_file(std::string_view filepath, uint64_t groupid) noexcept;
-    future<group_details> grp_detail(std::string_view groupname) noexcept;
+    future<> chown(std::string_view filepath, uid_t owner, gid_t group);
+    future<std::optional<struct group>> getgrnam(std::string_view name, char *buf, size_t buflen);
     future<uint64_t> file_size(std::string_view pathname) noexcept;
     future<bool> file_accessible(std::string_view pathname, access_flags flags) noexcept;
     future<bool> file_exists(std::string_view pathname) noexcept {

include/seastar/core/seastar.hh

@@ -47,6 +47,7 @@
 /// View the [Seastar compatibility statement](./md_compatibility.html) for
 /// information about library evolution.
 
+#include <grp.h>
 #include <seastar/core/sstring.hh>
 #include <seastar/core/future.hh>
 #include <seastar/core/file-types.hh>
@@ -81,7 +82,6 @@ enum class transport;
 class file;
 struct file_open_options;
 struct stat_data;
-struct group_details;
 
 namespace net {
 
@@ -360,17 +360,23 @@ using follow_symlink = bool_class<follow_symlink_tag>;
 /// with follow_symlink::yes, or for the link itself, with follow_symlink::no.
 future<stat_data> file_stat(std::string_view name, follow_symlink fs = follow_symlink::yes) noexcept;
 
-/// Returns details of the group from the group database.
+/// Returns details of the group from the group database. Wrapper around getgrnam_t.
+/// If the group is found, the std::optional contains the struct group information; otherwise, it is empty.
+/// The function throws std::error_code, when the getgrnam_t syscall fails.
 /// \param groupname name of the group
+/// \param buf buffer to store the string fields pointed to by the members of group structure.
+/// \param buflen size of the buffer
 ///
-/// \return group_details of the group identified by name.
-future<group_details> grp_detail(std::string_view groupname) noexcept;
+/// \return optional group_info of the group identified by name.
+future<std::optional<struct group>> getgrnam(std::string_view name, char *buf, size_t buflen);
 
-/// Change the owner and group of file. The owner is value computed after calling geteuid.
+/// Change the owner and group of file. This is a wrapper around chown syscall.
+/// The function throws std::system_error, when the chown syscall fails.
 /// \param filepath
-/// \param groupid
+/// \param owner
+/// \param group
 ///
-future<> change_ownership_of_file(std::string_view filepath, uint64_t groupid) noexcept;
+future<> chown(std::string_view filepath, uid_t owner, gid_t group);
 /// Return the size of a file.
 ///
 /// \param name name of the file to return the size

src/core/reactor.cc

@@ -2219,40 +2219,36 @@ void reactor::kill(pid_t pid, int sig) {
     ret.throw_if_error();
 }
 
-future<group_details> reactor::grp_detail(std::string_view groupname) noexcept {
-    return futurize_invoke([groupname, this] {
-        return _thread_pool->submit<syscall_result_extra<struct group>>([groupname = sstring(groupname)] {
-            struct group *gg = ::getgrnam(groupname.c_str());
-            if (!gg) {
-                return wrap_syscall(-1, *gg); // Simulating failure
+future<std::optional<struct group>> reactor::getgrnam(std::string_view name, char *buf, size_t buflen) {
+    return futurize_invoke([name, buf, buflen, this] {
+        return _thread_pool->submit<syscall_result_extra<std::optional<struct group>>>([name = sstring(name), buf, buflen] {
+            errno = 0;
+            struct group grp;
+            struct group *result;
+            memset(&grp, 0, sizeof(struct group));
+            int ret = ::getgrnam_r(name.c_str(), &grp, buf, buflen, &result);
+            if (!result) {
+                return wrap_syscall(ret, std::optional<struct group>(std::nullopt));
             }
-            return wrap_syscall(0, *gg);
-        }).then([groupname = sstring(groupname)] (syscall_result_extra<struct group> gr) {
-            gr.throw_fs_exception_if_error("group failed", groupname);
-            struct group& g = gr.extra;
-            group_details g_details;
-            g_details.group_name = g.gr_name;
-            g_details.group_password = g.gr_passwd;
-            g_details.group_gid = g.gr_gid;
-            g_details.group_member = g.gr_mem;
-            return make_ready_future<group_details>(std::move(g_details));
+            return wrap_syscall(ret, std::optional<struct group>(*result));
+        }).then([] (syscall_result_extra<std::optional<struct group>> sr) {
+            if (sr.result != 0) {
+                // getgrnam_r, in case of error, an error number is returned (here it is stored in result), 
+                // which is the same as the error code in errno
+                throw std::error_code(sr.result, std::system_category());
+            }
+            return make_ready_future<std::optional<struct group>>(std::optional<struct group>(std::move(sr.extra)));
         });
     });
 }
 
-future<> reactor::change_ownership_of_file(std::string_view filepath, uint64_t groupid) noexcept {
-    return futurize_invoke([filepath, groupid, this] {
-        return _thread_pool->submit<syscall_result<int>>([filepath = sstring(filepath), groupid] {
-            int chown_result = ::chown(filepath.c_str(), ::geteuid(), groupid);
+future<> reactor::chown(std::string_view filepath, uid_t owner, gid_t group) {
+    return futurize_invoke([filepath, owner, group, this] {
+        return _thread_pool->submit<syscall_result<int>>([filepath = sstring(filepath), owner, group] {
+            int chown_result = ::chown(filepath.c_str(), owner, group);
             return wrap_syscall(chown_result);
-        }).then([filepath = sstring(filepath), groupid] (syscall_result<int> sr) {
-            if (sr.result < 0) {
-                if (sr.error == EPERM) {
-                    sr.throw_fs_exception(format("Failed to change group of {}: Permission denied. Make sure the user has the root privilege or is a member of the group {}.", filepath, groupid), fs::path(filepath));
-                } else {
-                    sr.throw_fs_exception(format("Failed to chown {}: {} ()", filepath, strerror(sr.error)), fs::path(filepath));
-                }
-            }
+        }).then([] (syscall_result<int> sr) {
+            sr.throw_if_error();
             return make_ready_future<>();
         });
     });

src/util/file.cc

@@ -132,12 +132,12 @@ future<stat_data> file_stat(std::string_view name, follow_symlink follow) noexce
     return engine().file_stat(name, follow);
 }
 
-future<group_details> grp_detail(std::string_view groupname) noexcept {
-    return engine().grp_detail(groupname);
+future<std::optional<struct group>> getgrnam(std::string_view name, char *buf, size_t buflen) {
+    return engine().getgrnam(name, buf, buflen);
 }
 
-future<> change_ownership_of_file(std::string_view filepath, uint64_t groupgid) noexcept {
-    return engine().change_ownership_of_file(filepath, groupgid);
+future<> chown(std::string_view filepath, uid_t owner, gid_t group) {
+    return engine().chown(filepath, owner, group);
 }
 
 future<uint64_t> file_size(std::string_view name) noexcept {