Move error reporting to inside open_proc and friends

...and make it correctly print the file name we were unable to open.
Also, error from fdopen[dir]() is now reported with file name as well.

Note that open_proc() and friends need to be macros in order for
pr_perror() to show actual file name and line number where error occured.

Historical note: the original version of this patch was way more radical,
changing openat() to open() and thus removing pid_dir (replacing with pid
when needed) and open_proc_dir(), changing openat() to open(). The word
from Pavel is he wants to keep the openat/pid_dir optimization because
it saves two dentry lookups in kernel code for each open(). Because of
this optimization (and desire to print correct file name in case
of error) we have to carry both pid and pid_dir everywhere.
Signed-off-by: Kir Kolyshkin <kir@openvz.org>
Acked-by: Pavel Emelyanov <xemul@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>

cr-dump.c

@@ -138,29 +138,25 @@ err:
 	return ret;
 }
 
-static int dump_task_special_files(int pid_dir, struct cr_fdset *cr_fdset)
+static int dump_task_special_files(pid_t pid, int pid_dir, struct cr_fdset *cr_fdset)
 {
 	struct fd_parms params;
 	int fd, ret;
 
 	/* Dump /proc/pid/cwd */
 	params = (struct fd_parms){ .fd_name = FDINFO_CWD, };
-	fd = open_proc(pid_dir, "cwd");
-	if (fd < 0) {
-		pr_perror("Failed to openat cwd");
+	fd = open_proc(pid, pid_dir, "cwd");
+	if (fd < 0)
 		return -1;
-	}
 	ret = dump_one_reg_file(FDINFO_FD, &params, fd, cr_fdset, 1);
 	if (ret)
 		return ret;
 
 	/* Dump /proc/pid/exe */
 	params = (struct fd_parms){ .fd_name = FDINFO_EXE, };
-	fd = open_proc(pid_dir, "exe");
-	if (fd < 0) {
-		pr_perror("Failed to openat exe");
+	fd = open_proc(pid, pid_dir, "exe");
+	if (fd < 0)
 		return -1;
-	}
 	ret = dump_one_reg_file(FDINFO_FD, &params, fd, cr_fdset, 1);
 
 	return ret;
@@ -311,11 +307,9 @@ static int read_fd_params(pid_t pid, int pid_dir, char *fd, struct fd_parms *p)
 	FILE *file;
 	int ret;
 
-	file = fopen_proc(pid_dir, "fdinfo/%s", fd);
-	if (!file) {
-		pr_perror("Can't open %d's %s fdinfo", pid, fd);
+	file = fopen_proc(pid, pid_dir, "fdinfo/%s", fd);
+	if (!file)
 		return -1;
-	}
 
 	p->fd_name = atoi(fd);
 	ret = fscanf(file, "pos:\t%li\nflags:\t%o\nid:\t%s\n", &p->pos, &p->flags, p->id);
@@ -348,16 +342,14 @@ static int dump_task_files(pid_t pid, int pid_dir, struct cr_fdset *cr_fdset)
 	 * to re-read them in restorer, so better to make it
 	 * fast.
 	 */
-	if (dump_task_special_files(pid_dir, cr_fdset)) {
+	if (dump_task_special_files(pid, pid_dir, cr_fdset)) {
 		pr_err("Can't dump special files\n");
 		return -1;
 	}
 
-	fd_dir = opendir_proc(pid_dir, "fd");
-	if (!fd_dir) {
-		pr_perror("Can't open %d's fd", pid);
+	fd_dir = opendir_proc(pid, pid_dir, "fd");
+	if (!fd_dir)
 		return -1;
-	}
 
 	while ((de = readdir(fd_dir))) {
 		char id[FD_ID_SIZE];
@@ -455,7 +447,7 @@ static int dump_task_creds(pid_t pid, int pid_dir,
 	pr_info("Dumping creds for %d)\n", pid);
 	pr_info("----------------------------------------\n");
 
-	ret = parse_pid_status(pid_dir, &cr);
+	ret = parse_pid_status(pid, pid_dir, &cr);
 	if (ret < 0)
 		return ret;
 
@@ -497,11 +489,9 @@ static int get_task_sigmask(pid_t pid, int pid_dir, u64 *task_sigset)
 	/*
 	 * Now signals.
 	 */
-	file = fopen_proc(pid_dir, "status");
-	if (!file) {
-		pr_perror("Can't open %d status", pid);
+	file = fopen_proc(pid, pid_dir, "status");
+	if (!file)
 		goto err;
-	}
 
 	while (fgets(loc_buf, sizeof(loc_buf), file)) {
 		if (!strncmp(loc_buf, "SigBlk:", 7)) {
@@ -519,13 +509,11 @@ err:
 
 static int get_task_auxv(pid_t pid, int pid_dir, struct core_entry *core)
 {
-	int fd = open_proc(pid_dir, "auxv");
+	int fd = open_proc(pid, pid_dir, "auxv");
 	int ret, i;
 
-	if (fd < 0) {
-		pr_perror("Can't open %d's auxv", pid);
+	if (fd < 0)
 		return -1;
-	}
 
 	for (i = 0; i < AT_VECTOR_SIZE; i++) {
 		ret = read(fd, &core->tc.mm_saved_auxv[i],
@@ -552,11 +540,9 @@ static int get_task_personality(pid_t pid, int pid_dir, u32 *personality)
 	FILE *file = NULL;
 	int ret = -1;
 
-	file = fopen_proc(pid_dir, "personality");
-	if (!file) {
-		pr_perror("Can't open %d personality", pid);
+	file = fopen_proc(pid, pid_dir, "personality");
+	if (!file)
 		goto err;
-	}
 
 	if (!fgets(loc_buf, sizeof(loc_buf), file)) {
 		perror("Can't read task personality");
@@ -738,18 +724,16 @@ err:
 	return ret;
 }
 
-static int parse_threads(struct pstree_item *item, int pid_dir)
+static int parse_threads(struct pstree_item *item, pid_t pid, int pid_dir)
 {
 	struct dirent *de;
 	DIR *dir;
 	u32 *t = NULL;
 	int nr = 1;
 
-	dir = opendir_proc(pid_dir, "task");
-	if (!dir) {
-		pr_perror("Can't open %d/task", item->pid);
+	dir = opendir_proc(pid, pid_dir, "task");
+	if (!dir)
 		return -1;
-	}
 
 	while ((de = readdir(dir))) {
 		u32 *tmp;
@@ -776,7 +760,7 @@ static int parse_threads(struct pstree_item *item, int pid_dir)
 	return 0;
 }
 
-static int parse_children(struct pstree_item *item, int pid_dir)
+static int parse_children(struct pstree_item *item, pid_t pid, int pid_dir)
 {
 	FILE *file;
 	char *tok;
@@ -785,12 +769,9 @@ static int parse_children(struct pstree_item *item, int pid_dir)
 
 	for (i = 0; i < item->nr_threads; i++) {
 
-		file = fopen_proc(pid_dir, "task/%d/children", item->threads[i]);
-		if (!file) {
-			pr_perror("Can't open %d children %d",
-					item->pid, item->threads[i]);
+		file = fopen_proc(pid, pid_dir, "task/%d/children", item->threads[i]);
+		if (!file)
 			goto err;
-		}
 
 		if (!(fgets(loc_buf, sizeof(loc_buf), file)))
 			loc_buf[0] = 0;
@@ -918,7 +899,7 @@ static struct pstree_item *collect_task(pid_t pid, struct list_head *list)
 		item->state = TASK_DEAD;
 	}
 
-	ret = parse_threads(item, pid_dir);
+	ret = parse_threads(item, pid, pid_dir);
 	if (ret < 0)
 		goto err_close;
 
@@ -926,7 +907,7 @@ static struct pstree_item *collect_task(pid_t pid, struct list_head *list)
 	if (ret < 0)
 		goto err_close;
 
-	ret = parse_children(item, pid_dir);
+	ret = parse_children(item, pid, pid_dir);
 	if (ret < 0)
 		goto err_close;
 
@@ -1286,7 +1267,7 @@ int cr_dump_tasks(pid_t pid, struct cr_options *opts)
 	LIST_HEAD(pstree_list);
 	struct cr_fdset *cr_fdset = NULL;
 	struct pstree_item *item;
-	int i, ret = -1, pid_dir;
+	int i, ret = -1;
 
 	pr_info("========================================\n");
 	if (!opts->leader_only)

include/proc_parse.h

@@ -80,6 +80,6 @@ struct proc_status_creds {
 extern int parse_pid_stat(pid_t pid, int pid_dir, struct proc_pid_stat *s);
 extern int parse_pid_stat_small(pid_t pid, int pid_dir, struct proc_pid_stat_small *s);
 extern int parse_maps(pid_t pid, int pid_dir, struct list_head *vma_area_list, bool use_map_files);
-extern int parse_pid_status(int pid_dir, struct proc_status_creds *);
+extern int parse_pid_status(pid_t pid, int pid_dir, struct proc_status_creds *);
 
 #endif /* PROC_PARSE_H__ */

include/util.h

@@ -205,10 +205,58 @@ extern int reopen_fd_as_safe(int new_fd, int old_fd, bool allow_reuse_fd);
 extern void hex_dump(void *addr, unsigned long len);
 
 int open_pid_proc(pid_t pid);
-int open_proc(int pid_dir_fd, char *fmt, ...);
-int open_proc_rw(int pid_dir_fd, char *fmt, ...)
-DIR *opendir_proc(int pid_dir_fd, char *fmt, ...);
-FILE *fopen_proc(int pid_dir_fd, char *fmt, ...);
+int do_open_proc(int pid_dir, int flags, const char *fmt, ...);
+
+#define __open_proc(pid, pid_dir, flags, fmt, ...)		\
+	({							\
+		int __fd = do_open_proc(pid_dir, flags,		\
+					fmt, ##__VA_ARGS__);	\
+		if (__fd < 0)					\
+			pr_perror("Can't open /proc/%d/" fmt,	\
+					pid, ##__VA_ARGS__);	\
+								\
+		__fd;						\
+	})
+
+/* int open_proc(pid_t pid, int pid_dir, const char *fmt, ...); */
+#define open_proc(pid, pid_dir, fmt, ...)			\
+	__open_proc(pid, pid_dir, O_RDONLY, fmt, ##__VA_ARGS__)
+
+/* int open_proc_rw(pid_t pid, int pid_dir, const char *fmt, ...); */
+#define open_proc_rw(pid, pid_dir, fmt, ...)			\
+	__open_proc(pid, pid_dir, O_RDWR, fmt, ##__VA_ARGS__)
+
+/* DIR *opendir_proc(pid_t pid, int pid_dir,  const char *fmt, ...); */
+#define opendir_proc(pid, pid_dir, fmt, ...)				\
+	({								\
+		int __fd = open_proc(pid, pid_dir, fmt, ##__VA_ARGS__);	\
+		DIR *__d = NULL;					\
+									\
+		if (__fd >= 0)						\
+			__d = fdopendir(__fd);				\
+			if (__d == NULL)				\
+				pr_perror("Can't fdopendir %d "		\
+					"(/proc/%d/" fmt ")",		\
+					__fd, pid, ##__VA_ARGS__);	\
+									\
+		__d;							\
+	 })
+
+/* FILE *fopen_proc(pid_t pid, int pid_dir, const char *fmt, ...); */
+#define fopen_proc(pid, pid_dir, fmt, ...)				\
+	({								\
+		int __fd = open_proc(pid, pid_dir, fmt, ##__VA_ARGS__);	\
+		FILE *__f = NULL;					\
+									\
+		if (__fd >= 0)						\
+			__f = fdopen(__fd, "r");			\
+			if (__f == NULL)				\
+				pr_perror("Can't fdopen %d "		\
+					"(/proc/%d/" fmt ")",		\
+					__fd, pid, ##__VA_ARGS__);	\
+									\
+		__f;							\
+	 })
 
 #define __xalloc(op, size, ...)						\
 	({								\

parasite-syscall.c

@@ -620,7 +620,7 @@ struct parasite_ctl *parasite_infect_seized(pid_t pid, int pid_dir, struct list_
 
 	ctl->map_length = round_up(parasite_size, PAGE_SIZE);
 
-	fd = open_proc_rw(pid_dir, "map_files/%p-%p",
+	fd = open_proc_rw(pid, pid_dir, "map_files/%p-%p",
 		 ctl->remote_map, ctl->remote_map + ctl->map_length);
 	if (fd < 0) {
 		pr_perror("Can't open remote parasite map");

proc_parse.c

@@ -28,19 +28,15 @@ int parse_maps(pid_t pid, int pid_dir, struct list_head *vma_area_list, bool use
 	DIR *map_files_dir = NULL;
 	FILE *maps = NULL;
 
-	maps = fopen_proc(pid_dir, "maps");
-	if (!maps) {
-		pr_perror("Can't open %d's maps", pid);
+	maps = fopen_proc(pid, pid_dir, "maps");
+	if (!maps)
 		goto err;
-	}
 
 	if (use_map_files) {
-		map_files_dir = opendir_proc(pid_dir, "map_files");
-		if (!map_files_dir) {
-			pr_perror("Can't open %d's map_files (old kernel?)\n", pid);
+		map_files_dir = opendir_proc(pid, pid_dir, "map_files");
+		if (!map_files_dir) /* old kernel? */
 			goto err;
 	}
-	}
 
 	while (fgets(big_buffer, sizeof(big_buffer), maps)) {
 		struct stat st_buf;
@@ -189,11 +185,9 @@ int parse_pid_stat_small(pid_t pid, int pid_dir, struct proc_pid_stat_small *s)
 	char *tok;
 	int n;
 
-	f = fopen_proc(pid_dir, "stat");
-	if (f == NULL) {
-		pr_perror("Can't open %d's stat", pid);
+	f = fopen_proc(pid, pid_dir, "stat");
+	if (f == NULL)
 		return -1;
-	}
 
 	memset(s, 0, sizeof(*s));
 	n = fscanf(f, "%d " PROC_TASK_COMM_LEN_FMT " %c",
@@ -219,11 +213,9 @@ int parse_pid_stat(pid_t pid, int pid_dir, struct proc_pid_stat *s)
 	char *tok;
 	int n;
 
-	f = fopen_proc(pid_dir, "stat");
-	if (f == NULL) {
-		pr_perror("Can't open %d's stat", pid);
+	f = fopen_proc(pid, pid_dir, "stat");
+	if (f == NULL)
 		return -1;
-	}
 
 	memset(s, 0, sizeof(*s));
 	n = fscanf(f,
@@ -326,13 +318,13 @@ static int cap_parse(char *str, unsigned int *res)
 	return 0;
 }
 
-int parse_pid_status(int pid_dir, struct proc_status_creds *cr)
+int parse_pid_status(pid_t pid, int pid_dir, struct proc_status_creds *cr)
 {
 	int done = 0;
 	FILE *f;
 	char str[64];
 
-	f = fopen_proc(pid_dir, "status");
+	f = fopen_proc(pid, pid_dir, "status");
 	if (f == NULL) {
 		pr_perror("Can't open proc status");
 		return -1;

util.c

@@ -217,46 +217,14 @@ int open_pid_proc(pid_t pid)
 	return fd;
 }
 
-#define do_open_proc(pid_dir_fd, fmt, flags)			\
-	({							\
-		char fname[64];					\
-		va_list args;					\
-								\
-		va_start(args, fmt);				\
-		vsnprintf(fname, sizeof(fname), fmt, args);	\
-		va_end(args);					\
-								\
-		openat(pid_dir_fd, fname, flags);		\
-	})
-
-int open_proc(int pid_dir_fd, char *fmt, ...)
+int do_open_proc(int dirfd, int flags, const char *fmt, ...)
 {
-	return do_open_proc(pid_dir_fd, fmt, O_RDONLY);
-}
-
-int open_proc_rw(int pid_dir_fd, char *fmt, ...)
-{
-	return do_open_proc(pid_dir_fd, fmt, O_RDWR);
-}
-
-DIR *opendir_proc(int pid_dir_fd, char *fmt, ...)
-{
-	int dirfd;
-
-	dirfd = do_open_proc(pid_dir_fd, fmt);
-	if (dirfd >= 0)
-		return fdopendir(dirfd);
-
-	return NULL;
-}
-
-FILE *fopen_proc(int pid_dir_fd, char *fmt, ...)
-{
-	int fd;
+	char path[128];
+	va_list args;
 
-	fd = do_open_proc(pid_dir_fd, fmt);
-	if (fd >= 0)
-		return fdopen(fd, "r");
+	va_start(args, fmt);
+	vsnprintf(path, sizeof(path), fmt, args);
+	va_end(args);
 
-	return NULL;
+	return openat(dirfd, path, flags);
 }