The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxcfs/pull/349
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) === Signed-off-by: Christian Brauner <christian.brau...@ubuntu.com>
From f75d5b75d11e8dd9060155569f8c093b20ab8d37 Mon Sep 17 00:00:00 2001 From: Christian Brauner <christian.brau...@ubuntu.com> Date: Thu, 5 Mar 2020 11:02:22 +0100 Subject: [PATCH 1/4] proc_fuse: cleanup Signed-off-by: Christian Brauner <christian.brau...@ubuntu.com> --- src/proc_fuse.c | 172 ++++++++++++++++++++++-------------------------- 1 file changed, 77 insertions(+), 95 deletions(-) diff --git a/src/proc_fuse.c b/src/proc_fuse.c index 392d10d..5cb62c4 100644 --- a/src/proc_fuse.c +++ b/src/proc_fuse.c @@ -79,6 +79,7 @@ int proc_getattr(const char *path, struct stat *sb) memset(sb, 0, sizeof(struct stat)); if (clock_gettime(CLOCK_REALTIME, &now) < 0) return -EINVAL; + sb->st_uid = sb->st_gid = 0; sb->st_atim = sb->st_mtim = sb->st_ctim = now; if (strcmp(path, "/proc") == 0) { @@ -86,13 +87,14 @@ int proc_getattr(const char *path, struct stat *sb) sb->st_nlink = 2; return 0; } - if (strcmp(path, "/proc/meminfo") == 0 || - strcmp(path, "/proc/cpuinfo") == 0 || - strcmp(path, "/proc/uptime") == 0 || - strcmp(path, "/proc/stat") == 0 || - strcmp(path, "/proc/diskstats") == 0 || - strcmp(path, "/proc/swaps") == 0 || - strcmp(path, "/proc/loadavg") == 0) { + + if (strcmp(path, "/proc/meminfo") == 0 || + strcmp(path, "/proc/cpuinfo") == 0 || + strcmp(path, "/proc/uptime") == 0 || + strcmp(path, "/proc/stat") == 0 || + strcmp(path, "/proc/diskstats") == 0 || + strcmp(path, "/proc/swaps") == 0 || + strcmp(path, "/proc/loadavg") == 0) { sb->st_size = 0; sb->st_mode = S_IFREG | 00444; sb->st_nlink = 1; @@ -119,19 +121,19 @@ int proc_readdir(const char *path, void *buf, fuse_fill_dir_t filler, return 0; } -static off_t get_procfile_size(const char *which) +static off_t get_procfile_size(const char *path) { - FILE *f = fopen(which, "re"); - char *line = NULL; + __do_fclose FILE *f = NULL; + __do_free char *line = NULL; size_t len = 0; ssize_t sz, answer = 0; + + f = fopen(path, "re"); if (!f) return 0; while ((sz = getline(&line, &len, f)) != -1) answer += sz; - fclose (f); - free(line); return answer; } @@ -187,6 +189,7 @@ int proc_access(const char *path, int mask) /* these are all read-only */ if ((mask & ~R_OK) != 0) return -EACCES; + return 0; } @@ -198,9 +201,9 @@ int proc_release(const char *path, struct fuse_file_info *fi) static unsigned long get_memlimit(const char *cgroup, bool swap) { - int ret; __do_free char *memlimit_str = NULL; unsigned long memlimit = -1; + int ret; if (swap) ret = cgroup_ops->get_memory_swap_max(cgroup_ops, cgroup, &memlimit_str); @@ -219,6 +222,9 @@ static unsigned long get_min_memlimit(const char *cgroup, bool swap) unsigned long retlimit; copy = strdup(cgroup); + if (!copy) + return log_error_errno(0, ENOMEM, "Failed to allocate memory"); + retlimit = get_memlimit(copy, swap); while (strcmp(copy, "/") != 0) { @@ -233,11 +239,9 @@ static unsigned long get_min_memlimit(const char *cgroup, bool swap) return retlimit; } -static bool startswith(const char *line, const char *pref) +static inline bool startswith(const char *line, const char *pref) { - if (strncmp(line, pref, strlen(pref)) == 0) - return true; - return false; + return strncmp(line, pref, strlen(pref)) == 0; } static int proc_swaps_read(char *buf, size_t size, off_t offset, @@ -325,16 +329,16 @@ static int proc_swaps_read(char *buf, size_t size, off_t offset, total_len += l; } - if (total_len < 0 || l < 0) { - perror("Error writing to cache"); - return 0; - } + if (total_len < 0 || l < 0) + return log_error(0, "Failed writing to cache"); d->cached = 1; d->size = (int)total_len; - if (total_len > size) total_len = size; + if (total_len > size) + total_len = size; memcpy(buf, d->buf, total_len); + return total_len; } @@ -343,13 +347,13 @@ static void get_blkio_io_value(char *str, unsigned major, unsigned minor, { char *eol; char key[32]; + size_t len; memset(key, 0, 32); snprintf(key, 32, "%u:%u %s", major, minor, iotype); - size_t len = strlen(key); *v = 0; - + len = strlen(key); while (*str) { if (startswith(str, key)) { sscanf(str + len, "%lu", v); @@ -487,14 +491,11 @@ static int proc_diskstats_read(char *buf, size_t size, off_t offset, continue; l = snprintf(cache, cache_size, "%s", lbuf); - if (l < 0) { - perror("Error writing to fuse buf"); - return 0; - } - if (l >= cache_size) { - lxcfs_error("%s\n", "Internal error: truncated write to cache."); - return 0; - } + if (l < 0) + return log_error(0, "Failed to write cache"); + if (l >= cache_size) + return log_error(0, "Write to cache was truncated"); + cache += l; cache_size -= l; total_len += l; @@ -624,18 +625,12 @@ static double get_reaper_start_time_in_sec(pid_t pid) double res = 0; clockticks = get_reaper_start_time(pid); - if (clockticks == 0 && errno == EINVAL) { - lxcfs_debug("failed to retrieve start time of pid %d\n", pid); - return 0; - } + if (clockticks == 0 && errno == EINVAL) + return log_debug(0, "Failed to retrieve start time of pid %d", pid); ret = sysconf(_SC_CLK_TCK); - if (ret < 0 && errno == EINVAL) { - lxcfs_debug( - "%s\n", - "failed to determine number of clock ticks in a second"); - return 0; - } + if (ret < 0 && errno == EINVAL) + return log_debug(0, "Failed to determine number of clock ticks in a second"); ticks_per_sec = (uint64_t)ret; res = (double)clockticks / ticks_per_sec; @@ -692,19 +687,25 @@ static int proc_uptime_read(char *buf, size_t size, off_t offset, iwashere(); #endif - if (offset){ + if (offset) { + int left; + if (!d->cached) return 0; + if (offset > d->size) return -EINVAL; - int left = d->size - offset; - total_len = left > size ? size: left; + + left = d->size - offset; + total_len = left > size ? size : left; memcpy(buf, cache + offset, total_len); + return total_len; } reaperage = get_reaper_age(fc->pid); - /* To understand why this is done, please read the comment to the + /* + * To understand why this is done, please read the comment to the * get_reaper_busy() function. */ idletime = reaperage; @@ -712,15 +713,14 @@ static int proc_uptime_read(char *buf, size_t size, off_t offset, idletime = reaperage - busytime; total_len = snprintf(d->buf, d->buflen, "%.2lf %.2lf\n", reaperage, idletime); - if (total_len < 0 || total_len >= d->buflen){ - lxcfs_error("%s\n", "failed to write to cache"); - return 0; - } + if (total_len < 0 || total_len >= d->buflen) + return log_error(0, "Failed to write to cache"); d->size = (int)total_len; d->cached = 1; - if (total_len > size) total_len = size; + if (total_len > size) + total_len = size; memcpy(buf, d->buf, total_len); return total_len; @@ -800,10 +800,8 @@ static int proc_stat_read(char *buf, size_t size, off_t offset, return 0; //skip first line - if (getline(&line, &linelen, f) < 0) { - lxcfs_error("%s\n", "proc_stat_read read first line failed."); - return 0; - } + if (getline(&line, &linelen, f) < 0) + return log_error(0, "proc_stat_read read first line failed"); if (cgroup_ops->can_use_cpuview(cgroup_ops) && cg_cpu_usage) { total_len = cpuview_proc_stat(cg, cpuset, cg_cpu_usage, cg_cpu_usage_size, @@ -823,24 +821,24 @@ static int proc_stat_read(char *buf, size_t size, off_t offset, if (sscanf(line, "cpu%9[^ ]", cpu_char) != 1) { /* not a ^cpuN line containing a number N, just print it */ l = snprintf(cache, cache_size, "%s", line); - if (l < 0) { - perror("Error writing to cache"); - return 0; - } - if (l >= cache_size) { - lxcfs_error("%s\n", "Internal error: truncated write to cache."); - return 0; - } + if (l < 0) + return log_error(0, "Failed to write cache"); + if (l >= cache_size) + return log_error(0, "Write to cache was truncated"); + cache += l; cache_size -= l; total_len += l; + continue; } if (sscanf(cpu_char, "%d", &physcpu) != 1) continue; + if (!cpu_in_cpuset(physcpu, cpuset)) continue; + curcpu++; ret = sscanf(line, "%*s %lu %lu %lu %lu %lu %lu %lu %lu %lu %lu", @@ -854,21 +852,16 @@ static int proc_stat_read(char *buf, size_t size, off_t offset, &steal, &guest, &guest_nice); - if (ret != 10 || !cg_cpu_usage) { c = strchr(line, ' '); if (!c) continue; - l = snprintf(cache, cache_size, "cpu%d%s", curcpu, c); - if (l < 0) { - perror("Error writing to cache"); - return 0; - } - if (l >= cache_size) { - lxcfs_error("%s\n", "Internal error: truncated write to cache."); - return 0; - } + l = snprintf(cache, cache_size, "cpu%d%s", curcpu, c); + if (l < 0) + return log_error(0, "Failed to write cache"); + if (l >= cache_size) + return log_error(0, "Write to cache was truncated"); cache += l; cache_size -= l; @@ -898,16 +891,10 @@ static int proc_stat_read(char *buf, size_t size, off_t offset, "cpu%d %" PRIu64 " 0 %" PRIu64 " %" PRIu64 " 0 0 0 0 0 0\n", curcpu, cg_cpu_usage[physcpu].user, cg_cpu_usage[physcpu].system, new_idle); - - if (l < 0) { - perror("Error writing to cache"); - return 0; - - } - if (l >= cache_size) { - lxcfs_error("%s\n", "Internal error: truncated write to cache."); - return 0; - } + if (l < 0) + return log_error(0, "Failed to write cache"); + if (l >= cache_size) + return log_error(0, "Write to cache was truncated"); cache += l; cache_size -= l; @@ -916,7 +903,6 @@ static int proc_stat_read(char *buf, size_t size, off_t offset, user_sum += cg_cpu_usage[physcpu].user; system_sum += cg_cpu_usage[physcpu].system; idle_sum += new_idle; - } else { user_sum += user; nice_sum += nice; @@ -949,7 +935,7 @@ static int proc_stat_read(char *buf, size_t size, off_t offset, cache += cpuall_len; } else { /* shouldn't happen */ - lxcfs_error("proc_stat_read copy cpuall failed, cpuall_len=%d.", cpuall_len); + lxcfs_error("proc_stat_read copy cpuall failed, cpuall_len=%d", cpuall_len); cpuall_len = 0; } @@ -1235,15 +1221,10 @@ static int proc_meminfo_read(char *buf, size_t size, off_t offset, } l = snprintf(cache, cache_size, "%s", printme); - if (l < 0) { - perror("Error writing to cache"); - return 0; - - } - if (l >= cache_size) { - lxcfs_error("%s\n", "Internal error: truncated write to cache."); - return 0; - } + if (l < 0) + return log_error(0, "Failed to write cache"); + if (l >= cache_size) + return log_error(0, "Write to cache was truncated"); cache += l; cache_size -= l; @@ -1252,7 +1233,8 @@ static int proc_meminfo_read(char *buf, size_t size, off_t offset, d->cached = 1; d->size = total_len; - if (total_len > size ) total_len = size; + if (total_len > size) + total_len = size; memcpy(buf, d->buf, total_len); return total_len; From 285194776d0046f8a5b6c40afd8ce5b0592d18d3 Mon Sep 17 00:00:00 2001 From: Christian Brauner <christian.brau...@ubuntu.com> Date: Thu, 5 Mar 2020 11:22:34 +0100 Subject: [PATCH 2/4] proc_fuse: introduce and use fdopen_cached() Note that in contrast to libc's fdopen() it doesn't consume the fd it is passed. Signed-off-by: Christian Brauner <christian.brau...@ubuntu.com> --- src/proc_fuse.c | 5 ++-- src/utils.c | 79 ++++++++++++++++++++++++++++++++++++++++--------- src/utils.h | 1 + 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/src/proc_fuse.c b/src/proc_fuse.c index 5cb62c4..64092fe 100644 --- a/src/proc_fuse.c +++ b/src/proc_fuse.c @@ -958,6 +958,7 @@ static bool cgroup_parse_memory_stat(const char *cgroup, struct memory_stat *mst __do_close_prot_errno int fd = -EBADF; __do_fclose FILE *f = NULL; __do_free char *line = NULL; + __do_free void *fdopen_cache = NULL; bool unified; size_t len = 0; ssize_t linelen; @@ -966,11 +967,9 @@ static bool cgroup_parse_memory_stat(const char *cgroup, struct memory_stat *mst if (fd < 0) return false; - f = fdopen(fd, "re"); + f = fdopen_cached(fd, "re", &fdopen_cache); if (!f) return false; - /* Transferring ownership to fdopen(). */ - move_fd(fd); unified = pure_unified_layout(cgroup_ops); while ((linelen = getline(&line, &len, f)) != -1) { diff --git a/src/utils.c b/src/utils.c index 0bc99c1..3235eaa 100644 --- a/src/utils.c +++ b/src/utils.c @@ -429,38 +429,49 @@ static void *must_realloc(void *orig, size_t sz) return ret; } -static char *file_to_buf(const char *path, size_t *length) +static char *fd_to_buf(int fd, size_t *length) { - __do_close_prot_errno int fd = -EBADF; __do_free char *copy = NULL; - char buf[PATH_MAX]; if (!length) return NULL; - fd = open(path, O_RDONLY | O_CLOEXEC); - if (fd < 0) - return NULL; - *length = 0; for (;;) { - int n; + ssize_t bytes_read; + char buf[4096]; char *old = copy; - n = read_nointr(fd, buf, sizeof(buf)); - if (n < 0) + bytes_read = read_nointr(fd, buf, sizeof(buf)); + if (bytes_read < 0) return NULL; - if (!n) + + if (!bytes_read) break; - copy = must_realloc(old, (*length + n) * sizeof(*old)); - memcpy(copy + *length, buf, n); - *length += n; + copy = must_realloc(old, (*length + bytes_read) * sizeof(*old)); + memcpy(copy + *length, buf, bytes_read); + *length += bytes_read; } return move_ptr(copy); } +static char *file_to_buf(const char *path, size_t *length) +{ + __do_close_prot_errno int fd = -EBADF; + char buf[PATH_MAX]; + + if (!length) + return NULL; + + fd = open(path, O_RDONLY | O_CLOEXEC); + if (fd < 0) + return NULL; + + return fd_to_buf(fd, length); +} + FILE *fopen_cached(const char *path, const char *mode, void **caller_freed_buffer) { __do_free char *buf = NULL; @@ -477,3 +488,43 @@ FILE *fopen_cached(const char *path, const char *mode, void **caller_freed_buffe *caller_freed_buffer = move_ptr(buf); return f; } + +static int fd_cloexec(int fd, bool cloexec) +{ + int oflags, nflags; + + oflags = fcntl(fd, F_GETFD, 0); + if (oflags < 0) + return -errno; + + if (cloexec) + nflags = oflags | FD_CLOEXEC; + else + nflags = oflags & ~FD_CLOEXEC; + + if (nflags == oflags) + return 0; + + if (fcntl(fd, F_SETFD, nflags) < 0) + return -errno; + + return 0; +} + +FILE *fdopen_cached(int fd, const char *mode, void **caller_freed_buffer) +{ + __do_free char *buf = NULL; + size_t len = 0; + FILE *f; + + buf = fd_to_buf(fd, &len); + if (!buf) + return NULL; + + f = fmemopen(buf, len, mode); + if (!f) + return NULL; + + *caller_freed_buffer = move_ptr(buf); + return f; +} diff --git a/src/utils.h b/src/utils.h index 7450fb6..05b9a42 100644 --- a/src/utils.h +++ b/src/utils.h @@ -73,5 +73,6 @@ static inline int pidfd_send_signal(int pidfd, int sig, siginfo_t *info, extern FILE *fopen_cached(const char *path, const char *mode, void **caller_freed_buffer); +extern FILE *fdopen_cached(int fd, const char *mode, void **caller_freed_buffer); #endif /* __LXCFS_UTILS_H */ From 965cb950f70348a333f2d2ac52c140498a265adc Mon Sep 17 00:00:00 2001 From: Christian Brauner <christian.brau...@ubuntu.com> Date: Thu, 5 Mar 2020 11:23:29 +0100 Subject: [PATCH 3/4] cgroup_utils: only transfer ownership of fd after fdopen() has succeeded Signed-off-by: Christian Brauner <christian.brau...@ubuntu.com> --- src/cgroups/cgroup_utils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cgroups/cgroup_utils.c b/src/cgroups/cgroup_utils.c index af9b403..7bdf65b 100644 --- a/src/cgroups/cgroup_utils.c +++ b/src/cgroups/cgroup_utils.c @@ -684,10 +684,11 @@ char *readat_file(int dirfd, const char *path) if (fd < 0) return NULL; - /* transfer ownership of fd */ - f = fdopen(move_fd(fd), "re"); + f = fdopen(fd, "re"); if (!f) return NULL; + /* Transfer ownership of fd */ + move_fd(fd); while ((linelen = getline(&line, &len, f)) != -1) { append_line(&buf, fulllen, line, linelen); From 9b817e41497e2a52691ab187c41c7a485fa2a757 Mon Sep 17 00:00:00 2001 From: Christian Brauner <christian.brau...@ubuntu.com> Date: Thu, 5 Mar 2020 11:24:38 +0100 Subject: [PATCH 4/4] proc_loadavg: use fdopen_cached() Signed-off-by: Christian Brauner <christian.brau...@ubuntu.com> --- src/proc_loadavg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/proc_loadavg.c b/src/proc_loadavg.c index e48fcab..6ca5d06 100644 --- a/src/proc_loadavg.c +++ b/src/proc_loadavg.c @@ -271,6 +271,7 @@ int proc_loadavg_read(char *buf, size_t size, off_t offset, static int calc_pid(char ***pid_buf, char *dpath, int depth, int sum, int cfd) { __do_free char *path = NULL; + __do_free void *fdopen_cache = NULL; __do_close_prot_errno int fd = -EBADF; __do_fclose FILE *f = NULL; __do_closedir DIR *dir = NULL; @@ -322,7 +323,7 @@ static int calc_pid(char ***pid_buf, char *dpath, int depth, int sum, int cfd) if (fd < 0) return sum; - f = fdopen(move_fd(fd), "r"); + f = fdopen_cached(fd, "re", &fdopen_cache); if (!f) return sum;
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel