From d6b0d375490f9c31da0131582b7d77d1fdd79706 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Thu, 6 Oct 2016 12:51:20 -0700 Subject: [PATCH] Small clean up of fuse code. - Add TEMP_FAILURE_RETRY around all relevant system calls. - Cleanup some of the read calls. - Add error log messages when read/write calls do not actually read/write the expected number of bytes. - Add error messages for write failures in fuse_status/fuse_replay. Test: Attached to the sdcard process and stepped through most of the modified Test: code. Also, create/read/write/delete files in /storage/emulated/0 Test: directory. Change-Id: I73e4c0db861960f4c0af1bf96b06cd61fa74be69 --- sdcard/fuse.cpp | 78 ++++++++++++++++++++++++++++------------------- sdcard/sdcard.cpp | 26 ++++++++-------- 2 files changed, 60 insertions(+), 44 deletions(-) diff --git a/sdcard/fuse.cpp b/sdcard/fuse.cpp index f549606a9..d4c51fd74 100644 --- a/sdcard/fuse.cpp +++ b/sdcard/fuse.cpp @@ -14,6 +14,10 @@ * limitations under the License. */ +#include +#include +#include + #define LOG_TAG "sdcard" #include "fuse.h" @@ -223,7 +227,8 @@ static void attr_from_stat(struct fuse* fuse, struct fuse_attr *attr, } static int touch(char* path, mode_t mode) { - int fd = open(path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW, mode); + int fd = TEMP_FAILURE_RETRY(open(path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC, + mode)); if (fd == -1) { if (errno == EEXIST) { return 0; @@ -469,27 +474,34 @@ static void fuse_status(struct fuse *fuse, __u64 unique, int err) hdr.len = sizeof(hdr); hdr.error = err; hdr.unique = unique; - write(fuse->fd, &hdr, sizeof(hdr)); + ssize_t ret = TEMP_FAILURE_RETRY(write(fuse->fd, &hdr, sizeof(hdr))); + if (ret == -1) { + PLOG(ERROR) << "*** STATUS FAILED ***"; + } else if (static_cast(ret) != sizeof(hdr)) { + LOG(ERROR) << "*** STATUS FAILED: written " << ret << " expected " + << sizeof(hdr) << " ***"; + } } static void fuse_reply(struct fuse *fuse, __u64 unique, void *data, int len) { struct fuse_out_header hdr; - struct iovec vec[2]; - int res; - hdr.len = len + sizeof(hdr); hdr.error = 0; hdr.unique = unique; + struct iovec vec[2]; vec[0].iov_base = &hdr; vec[0].iov_len = sizeof(hdr); vec[1].iov_base = data; vec[1].iov_len = len; - res = writev(fuse->fd, vec, 2); - if (res < 0) { + ssize_t ret = TEMP_FAILURE_RETRY(writev(fuse->fd, vec, 2)); + if (ret == -1) { PLOG(ERROR) << "*** REPLY FAILED ***"; + } else if (static_cast(ret) != sizeof(hdr) + len) { + LOG(ERROR) << "*** REPLY FAILED: written " << ret << " expected " + << sizeof(hdr) + len << " ***"; } } @@ -501,7 +513,7 @@ static int fuse_reply_entry(struct fuse* fuse, __u64 unique, struct fuse_entry_out out; struct stat s; - if (lstat(path, &s) < 0) { + if (lstat(path, &s) == -1) { return -errno; } @@ -528,7 +540,7 @@ static int fuse_reply_attr(struct fuse* fuse, __u64 unique, const struct node* n struct fuse_attr_out out; struct stat s; - if (lstat(path, &s) < 0) { + if (lstat(path, &s) == -1) { return -errno; } memset(&out, 0, sizeof(out)); @@ -542,10 +554,7 @@ static void fuse_notify_delete(struct fuse* fuse, const __u64 parent, const __u64 child, const char* name) { struct fuse_out_header hdr; struct fuse_notify_delete_out data; - struct iovec vec[3]; size_t namelen = strlen(name); - int res; - hdr.len = sizeof(hdr) + sizeof(data) + namelen + 1; hdr.error = FUSE_NOTIFY_DELETE; hdr.unique = 0; @@ -555,6 +564,7 @@ static void fuse_notify_delete(struct fuse* fuse, const __u64 parent, data.namelen = namelen; data.padding = 0; + struct iovec vec[3]; vec[0].iov_base = &hdr; vec[0].iov_len = sizeof(hdr); vec[1].iov_base = &data; @@ -562,10 +572,15 @@ static void fuse_notify_delete(struct fuse* fuse, const __u64 parent, vec[2].iov_base = (void*) name; vec[2].iov_len = namelen + 1; - res = writev(fuse->fd, vec, 3); + ssize_t ret = TEMP_FAILURE_RETRY(writev(fuse->fd, vec, 3)); /* Ignore ENOENT, since other views may not have seen the entry */ - if (res < 0 && errno != ENOENT) { - PLOG(ERROR) << "*** NOTIFY FAILED ***"; + if (ret == -1) { + if (errno != ENOENT) { + PLOG(ERROR) << "*** NOTIFY FAILED ***"; + } + } else if (static_cast(ret) != sizeof(hdr) + sizeof(data) + namelen + 1) { + LOG(ERROR) << "*** NOTIFY FAILED: written " << ret << " expected " + << sizeof(hdr) + sizeof(data) + namelen + 1 << " ***"; } } @@ -665,7 +680,7 @@ static int handle_setattr(struct fuse* fuse, struct fuse_handler* handler, /* XXX: incomplete implementation on purpose. * chmod/chown should NEVER be implemented.*/ - if ((req->valid & FATTR_SIZE) && truncate64(path, req->size) < 0) { + if ((req->valid & FATTR_SIZE) && TEMP_FAILURE_RETRY(truncate64(path, req->size)) == -1) { return -errno; } @@ -727,7 +742,7 @@ static int handle_mknod(struct fuse* fuse, struct fuse_handler* handler, return -EACCES; } __u32 mode = (req->mode & (~0777)) | 0664; - if (mknod(child_path, mode, req->rdev) < 0) { + if (mknod(child_path, mode, req->rdev) == -1) { return -errno; } return fuse_reply_entry(fuse, hdr->unique, parent_node, name, actual_name, child_path); @@ -757,7 +772,7 @@ static int handle_mkdir(struct fuse* fuse, struct fuse_handler* handler, return -EACCES; } __u32 mode = (req->mode & (~0777)) | 0775; - if (mkdir(child_path, mode) < 0) { + if (mkdir(child_path, mode) == -1) { return -errno; } @@ -804,7 +819,7 @@ static int handle_unlink(struct fuse* fuse, struct fuse_handler* handler, if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK)) { return -EACCES; } - if (unlink(child_path) < 0) { + if (unlink(child_path) == -1) { return -errno; } pthread_mutex_lock(&fuse->global->lock); @@ -854,7 +869,7 @@ static int handle_rmdir(struct fuse* fuse, struct fuse_handler* handler, if (!check_caller_access_to_name(fuse, hdr, parent_node, name, W_OK)) { return -EACCES; } - if (rmdir(child_path) < 0) { + if (rmdir(child_path) == -1) { return -errno; } pthread_mutex_lock(&fuse->global->lock); @@ -942,7 +957,7 @@ static int handle_rename(struct fuse* fuse, struct fuse_handler* handler, DLOG(INFO) << "[" << handler->token << "] RENAME " << old_child_path << "->" << new_child_path; res = rename(old_child_path, new_child_path); - if (res < 0) { + if (res == -1) { res = -errno; goto io_error; } @@ -1004,8 +1019,8 @@ static int handle_open(struct fuse* fuse, struct fuse_handler* handler, return -ENOMEM; } DLOG(INFO) << "[" << handler->token << "] OPEN " << path; - h->fd = open(path, req->flags); - if (h->fd < 0) { + h->fd = TEMP_FAILURE_RETRY(open(path, req->flags)); + if (h->fd == -1) { free(h); return -errno; } @@ -1035,8 +1050,8 @@ static int handle_read(struct fuse* fuse, struct fuse_handler* handler, if (size > MAX_READ) { return -EINVAL; } - res = pread64(h->fd, read_buffer, size, offset); - if (res < 0) { + res = TEMP_FAILURE_RETRY(pread64(h->fd, read_buffer, size, offset)); + if (res == -1) { return -errno; } fuse_reply(fuse, unique, read_buffer, res); @@ -1059,8 +1074,8 @@ static int handle_write(struct fuse* fuse, struct fuse_handler* handler, DLOG(INFO) << "[" << handler->token << "] WRITE " << std::hex << h << std::dec << "(" << h->fd << ") " << req->size << "@" << req->offset; - res = pwrite64(h->fd, buffer, req->size, req->offset); - if (res < 0) { + res = TEMP_FAILURE_RETRY(pwrite64(h->fd, buffer, req->size, req->offset)); + if (res == -1) { return -errno; } out.size = res; @@ -1084,7 +1099,7 @@ static int handle_statfs(struct fuse* fuse, struct fuse_handler* handler, if (res < 0) { return -ENOENT; } - if (statfs(fuse->global->root.name, &stat) < 0) { + if (TEMP_FAILURE_RETRY(statfs(fuse->global->root.name, &stat)) == -1) { return -errno; } memset(&out, 0, sizeof(out)); @@ -1293,7 +1308,6 @@ static int handle_canonical_path(struct fuse* fuse, struct fuse_handler* handler return NO_STATUS; } - static int handle_fuse_request(struct fuse *fuse, struct fuse_handler* handler, const struct fuse_in_header *hdr, const void *data, size_t data_len) { @@ -1427,7 +1441,7 @@ void handle_fuse_requests(struct fuse_handler* handler) for (;;) { ssize_t len = TEMP_FAILURE_RETRY(read(fuse->fd, handler->request_buffer, sizeof(handler->request_buffer))); - if (len < 0) { + if (len == -1) { if (errno == ENODEV) { LOG(ERROR) << "[" << handler->token << "] someone stole our marbles!"; exit(2); @@ -1436,14 +1450,14 @@ void handle_fuse_requests(struct fuse_handler* handler) continue; } - if ((size_t)len < sizeof(struct fuse_in_header)) { + if (static_cast(len) < sizeof(struct fuse_in_header)) { LOG(ERROR) << "[" << handler->token << "] request too short: len=" << len; continue; } const struct fuse_in_header* hdr = reinterpret_cast(handler->request_buffer); - if (hdr->len != (size_t)len) { + if (hdr->len != static_cast(len)) { LOG(ERROR) << "[" << handler->token << "] malformed header: len=" << len << ", hdr->len=" << hdr->len; continue; diff --git a/sdcard/sdcard.cpp b/sdcard/sdcard.cpp index 70bbf2641..bc502a0ec 100644 --- a/sdcard/sdcard.cpp +++ b/sdcard/sdcard.cpp @@ -111,7 +111,7 @@ static void watch_package_list(struct fuse_global* global) { char event_buf[512]; int nfd = inotify_init(); - if (nfd < 0) { + if (nfd == -1) { PLOG(ERROR) << "inotify_init failed"; return; } @@ -142,17 +142,19 @@ static void watch_package_list(struct fuse_global* global) { } int event_pos = 0; - int res = read(nfd, event_buf, sizeof(event_buf)); - if (res < (int) sizeof(*event)) { - if (errno == EINTR) - continue; + ssize_t res = TEMP_FAILURE_RETRY(read(nfd, event_buf, sizeof(event_buf))); + if (res == -1) { PLOG(ERROR) << "failed to read inotify event"; return; + } else if (static_cast(res) < sizeof(*event)) { + LOG(ERROR) << "failed to read inotify event: read " << res << " expected " + << sizeof(event_buf); + return; } - while (res >= (int) sizeof(*event)) { + while (res >= static_cast(sizeof(*event))) { int event_size; - event = (struct inotify_event *) (event_buf + event_pos); + event = reinterpret_cast(event_buf + event_pos); DLOG(INFO) << "inotify event: " << std::hex << event->mask << std::dec; if ((event->mask & IN_IGNORED) == IN_IGNORED) { @@ -171,7 +173,7 @@ static void watch_package_list(struct fuse_global* global) { static int fuse_setup(struct fuse* fuse, gid_t gid, mode_t mask) { char opts[256]; - fuse->fd = open("/dev/fuse", O_RDWR); + fuse->fd = TEMP_FAILURE_RETRY(open("/dev/fuse", O_RDWR | O_CLOEXEC)); if (fuse->fd == -1) { PLOG(ERROR) << "failed to open fuse device"; return -1; @@ -182,8 +184,8 @@ static int fuse_setup(struct fuse* fuse, gid_t gid, mode_t mask) { snprintf(opts, sizeof(opts), "fd=%i,rootmode=40000,default_permissions,allow_other,user_id=%d,group_id=%d", fuse->fd, fuse->global->uid, fuse->global->gid); - if (mount("/dev/fuse", fuse->dest_path, "fuse", MS_NOSUID | MS_NODEV | MS_NOEXEC | - MS_NOATIME, opts) != 0) { + if (mount("/dev/fuse", fuse->dest_path, "fuse", MS_NOSUID | MS_NODEV | MS_NOEXEC | MS_NOATIME, + opts) == -1) { PLOG(ERROR) << "failed to mount fuse filesystem"; return -1; } @@ -321,7 +323,7 @@ static bool sdcardfs_setup(const std::string& source_path, const std::string& de fsuid, fsgid, multi_user?"multiuser,":"", mask, userid, gid); if (mount(source_path.c_str(), dest_path.c_str(), "sdcardfs", - MS_NOSUID | MS_NODEV | MS_NOEXEC | MS_NOATIME, opts.c_str()) != 0) { + MS_NOSUID | MS_NODEV | MS_NOEXEC | MS_NOATIME, opts.c_str()) == -1) { PLOG(ERROR) << "failed to mount sdcardfs filesystem"; return false; } @@ -480,7 +482,7 @@ int main(int argc, char **argv) { rlim.rlim_cur = 8192; rlim.rlim_max = 8192; - if (setrlimit(RLIMIT_NOFILE, &rlim)) { + if (setrlimit(RLIMIT_NOFILE, &rlim) == -1) { PLOG(ERROR) << "setting RLIMIT_NOFILE failed"; }