From 7eebe8a9c51686927709a57b1f2725d371014abc Mon Sep 17 00:00:00 2001 From: Jeff Dike Date: Fri, 6 Jan 2006 00:19:01 -0800 Subject: [PATCH] [PATCH] uml: umid cleanup This patch cleans up the umid code: - The only_if_set argument to get_umid is gone. - get_umid returns an empty string rather than NULL if there is no umid. - umid_is_random is gone since its users went away. - Some printfs were turned into printks because the code runs late enough that printk is working. - Error paths were cleaned up. - Some functions now return an error and let the caller print the error message rather than printing it themselves. This eliminates the practice of passing a pointer to printf or printk in, depending on where in the boot process we are. - Major tidying of not_dead_yet - mostly error path cleanup, plus a comment explaining why it doesn't react to errors the way you might expect. - Calls to os_* interfaces that were moved under os are changed back to their native libc forms. - snprintf, strlcpy, and their bounds-checking friends are used more often, replacing by-hand bounds checking in some places. Signed-off-by: Jeff Dike Cc: Paolo 'Blaisorblade' Giarrusso Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- arch/um/drivers/line.c | 4 +- arch/um/include/os.h | 4 +- arch/um/include/user_util.h | 1 - arch/um/kernel/um_arch.c | 4 +- arch/um/kernel/umid.c | 12 +- arch/um/os-Linux/umid.c | 265 +++++++++++++++++++++--------------- 6 files changed, 164 insertions(+), 126 deletions(-) diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c index b8e3e800ee4..a3c39373adb 100644 --- a/arch/um/drivers/line.c +++ b/arch/um/drivers/line.c @@ -831,8 +831,8 @@ char *add_xterm_umid(char *base) char *umid, *title; int len; - umid = get_umid(1); - if(umid == NULL) + umid = get_umid(); + if(*umid == '\0') return base; len = strlen(base) + strlen(" ()") + strlen(umid) + 1; diff --git a/arch/um/include/os.h b/arch/um/include/os.h index 258444e5b9b..c279ee6d89e 100644 --- a/arch/um/include/os.h +++ b/arch/um/include/os.h @@ -216,7 +216,7 @@ extern int helper_wait(int pid); /* umid.c */ extern int umid_file_name(char *name, char *buf, int len); -extern int set_umid(char *name, int (*printer)(const char *fmt, ...)); -extern char *get_umid(int only_if_set); +extern int set_umid(char *name); +extern char *get_umid(void); #endif diff --git a/arch/um/include/user_util.h b/arch/um/include/user_util.h index bb505e01d99..b9984003e60 100644 --- a/arch/um/include/user_util.h +++ b/arch/um/include/user_util.h @@ -64,7 +64,6 @@ extern void setup_machinename(char *machine_out); extern void setup_hostinfo(void); extern void do_exec(int old_pid, int new_pid); extern void tracer_panic(char *msg, ...); -extern char *get_umid(int only_if_set); extern void do_longjmp(void *p, int val); extern int detach(int pid, int sig); extern int attach(int pid); diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c index 142a9493912..26626b2b917 100644 --- a/arch/um/kernel/um_arch.c +++ b/arch/um/kernel/um_arch.c @@ -146,8 +146,8 @@ void set_cmdline(char *cmd) if(CHOOSE_MODE(honeypot, 0)) return; - umid = get_umid(1); - if(umid != NULL){ + umid = get_umid(); + if(*umid != '\0'){ snprintf(argv1_begin, (argv1_end - argv1_begin) * sizeof(*ptr), "(%s) ", umid); diff --git a/arch/um/kernel/umid.c b/arch/um/kernel/umid.c index 772c7cfbd8e..4eaee823bfd 100644 --- a/arch/um/kernel/umid.c +++ b/arch/um/kernel/umid.c @@ -3,15 +3,13 @@ * Licensed under the GPL */ -#include "linux/stddef.h" -#include "linux/kernel.h" #include "asm/errno.h" #include "init.h" #include "os.h" #include "kern.h" +#include "linux/kernel.h" -/* Changed by set_umid_arg and umid_file_name */ -int umid_is_random = 0; +/* Changed by set_umid_arg */ static int umid_inited = 0; static int __init set_umid_arg(char *name, int *add) @@ -22,11 +20,9 @@ static int __init set_umid_arg(char *name, int *add) return 0; *add = 0; - err = set_umid(name, printf); - if(err == -EEXIST){ + err = set_umid(name); + if(err == -EEXIST) printf("umid '%s' already in use\n", name); - umid_is_random = 1; - } else if(!err) umid_inited = 1; diff --git a/arch/um/os-Linux/umid.c b/arch/um/os-Linux/umid.c index 77d69a348cf..ecf107ae5ac 100644 --- a/arch/um/os-Linux/umid.c +++ b/arch/um/os-Linux/umid.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include "init.h" @@ -25,15 +26,16 @@ static char *uml_dir = UML_DIR; static int __init make_uml_dir(void) { char dir[512] = { '\0' }; - int len; + int len, err; if(*uml_dir == '~'){ char *home = getenv("HOME"); + err = -ENOENT; if(home == NULL){ - printf("make_uml_dir : no value in environment for " + printk("make_uml_dir : no value in environment for " "$HOME\n"); - exit(1); + goto err; } strlcpy(dir, home, sizeof(dir)); uml_dir++; @@ -43,18 +45,26 @@ static int __init make_uml_dir(void) if (len > 0 && dir[len - 1] != '/') strlcat(dir, "/", sizeof(dir)); + err = -ENOMEM; uml_dir = malloc(strlen(dir) + 1); if (uml_dir == NULL) { printf("make_uml_dir : malloc failed, errno = %d\n", errno); - exit(1); + goto err; } strcpy(uml_dir, dir); if((mkdir(uml_dir, 0777) < 0) && (errno != EEXIST)){ printf("Failed to mkdir '%s': %s\n", uml_dir, strerror(errno)); - return(-1); + err = -errno; + goto err_free; } return 0; + +err_free: + free(uml_dir); +err: + uml_dir = NULL; + return err; } static int actually_do_remove(char *dir) @@ -65,75 +75,88 @@ static int actually_do_remove(char *dir) char file[256]; directory = opendir(dir); - if(directory == NULL){ - printk("actually_do_remove : couldn't open directory '%s', " - "errno = %d\n", dir, errno); - return(1); - } + if(directory == NULL) + return -errno; + while((ent = readdir(directory)) != NULL){ if(!strcmp(ent->d_name, ".") || !strcmp(ent->d_name, "..")) continue; len = strlen(dir) + sizeof("/") + strlen(ent->d_name) + 1; - if(len > sizeof(file)){ - printk("Not deleting '%s' from '%s' - name too long\n", - ent->d_name, dir); - continue; - } + if(len > sizeof(file)) + return -E2BIG; + sprintf(file, "%s/%s", dir, ent->d_name); - if(unlink(file) < 0){ - printk("actually_do_remove : couldn't remove '%s' " - "from '%s', errno = %d\n", ent->d_name, dir, - errno); - return(1); - } - } - if(rmdir(dir) < 0){ - printk("actually_do_remove : couldn't rmdir '%s', " - "errno = %d\n", dir, errno); - return(1); + if(unlink(file) < 0) + return -errno; } - return(0); + if(rmdir(dir) < 0) + return -errno; + + return 0; } -extern int tracing_pid; +/* This says that there isn't already a user of the specified directory even if + * there are errors during the checking. This is because if these errors + * happen, the directory is unusable by the pre-existing UML, so we might as + * well take it over. This could happen either by + * the existing UML somehow corrupting its umid directory + * something other than UML sticking stuff in the directory + * this boot racing with a shutdown of the other UML + * In any of these cases, the directory isn't useful for anything else. + */ static int not_dead_yet(char *dir) { char file[strlen(uml_dir) + UMID_LEN + sizeof("/pid\0")]; char pid[sizeof("nnnnn\0")], *end; - int dead, fd, p, n; + int dead, fd, p, n, err; + + n = snprintf(file, sizeof(file), "%s/pid", dir); + if(n >= sizeof(file)){ + printk("not_dead_yet - pid filename too long\n"); + err = -E2BIG; + goto out; + } - sprintf(file, "%s/pid", dir); dead = 0; - fd = os_open_file(file, of_read(OPENFLAGS()), 0); + fd = open(file, O_RDONLY); if(fd < 0){ if(fd != -ENOENT){ printk("not_dead_yet : couldn't open pid file '%s', " "err = %d\n", file, -fd); - return(1); } - dead = 1; + goto out; } - if(fd > 0){ - n = os_read_file(fd, pid, sizeof(pid)); - if(n < 0){ - printk("not_dead_yet : couldn't read pid file '%s', " - "err = %d\n", file, -n); - return(1); - } - p = strtoul(pid, &end, 0); - if(end == pid){ - printk("not_dead_yet : couldn't parse pid file '%s', " - "errno = %d\n", file, errno); - dead = 1; - } - if(((kill(p, 0) < 0) && (errno == ESRCH)) || - (p == CHOOSE_MODE(tracing_pid, os_getpid()))) - dead = 1; + + err = 0; + n = read(fd, pid, sizeof(pid)); + if(n <= 0){ + printk("not_dead_yet : couldn't read pid file '%s', " + "err = %d\n", file, -n); + goto out_close; + } + + p = strtoul(pid, &end, 0); + if(end == pid){ + printk("not_dead_yet : couldn't parse pid file '%s', " + "errno = %d\n", file, errno); + goto out_close; } - if(!dead) - return(1); - return(actually_do_remove(dir)); + + if((kill(p, 0) == 0) || (errno != ESRCH)) + return 1; + + err = actually_do_remove(dir); + if(err) + printk("not_dead_yet - actually_do_remove failed with " + "err = %d\n", err); + + return err; + + out_close: + close(fd); + out: + return 0; } static void __init create_pid_file(void) @@ -145,26 +168,26 @@ static void __init create_pid_file(void) if(umid_file_name("pid", file, sizeof(file))) return; - fd = os_open_file(file, of_create(of_excl(of_rdwr(OPENFLAGS()))), - 0644); + fd = open(file, O_RDWR | O_CREAT | O_EXCL, 0644); if(fd < 0){ - printf("Open of machine pid file \"%s\" failed: %s\n", + printk("Open of machine pid file \"%s\" failed: %s\n", file, strerror(-fd)); return; } - sprintf(pid, "%d\n", os_getpid()); - n = os_write_file(fd, pid, strlen(pid)); + snprintf(pid, sizeof(pid), "%d\n", getpid()); + n = write(fd, pid, strlen(pid)); if(n != strlen(pid)) - printf("Write of pid file failed - err = %d\n", -n); - os_close_file(fd); + printk("Write of pid file failed - err = %d\n", -n); + + close(fd); } -int __init set_umid(char *name, int (*printer)(const char *fmt, ...)) +int __init set_umid(char *name) { if(strlen(name) > UMID_LEN - 1) - (*printer)("Unique machine name is being truncated to %d " - "characters\n", UMID_LEN); + return -E2BIG; + strlcpy(umid, name, sizeof(umid)); return 0; @@ -172,44 +195,56 @@ int __init set_umid(char *name, int (*printer)(const char *fmt, ...)) static int umid_setup = 0; -int __init make_umid(int (*printer)(const char *fmt, ...)) +int __init make_umid(void) { int fd, err; char tmp[256]; + if(umid_setup) + return 0; + make_uml_dir(); if(*umid == '\0'){ strlcpy(tmp, uml_dir, sizeof(tmp)); - strcat(tmp, "XXXXXX"); + strlcat(tmp, "XXXXXX", sizeof(tmp)); fd = mkstemp(tmp); if(fd < 0){ - (*printer)("make_umid - mkstemp(%s) failed: %s\n", - tmp,strerror(errno)); - return(1); + printk("make_umid - mkstemp(%s) failed: %s\n", + tmp, strerror(errno)); + err = -errno; + goto err; } - os_close_file(fd); + close(fd); + + set_umid(&tmp[strlen(uml_dir)]); + /* There's a nice tiny little race between this unlink and * the mkdir below. It'd be nice if there were a mkstemp * for directories. */ - unlink(tmp); - set_umid(&tmp[strlen(uml_dir)], printer); + if(unlink(tmp)){ + err = -errno; + goto err; + } } - sprintf(tmp, "%s%s", uml_dir, umid); + snprintf(tmp, sizeof(tmp), "%s%s", uml_dir, umid); err = mkdir(tmp, 0777); if(err < 0){ - if(errno == EEXIST){ - if(not_dead_yet(tmp)) - return -EEXIST; - err = mkdir(tmp, 0777); - } + err = -errno; + if(errno != EEXIST) + goto err; + + if(not_dead_yet(tmp) < 0) + goto err; + + err = mkdir(tmp, 0777); } if(err < 0){ - (*printer)("Failed to create %s - errno = %d\n", umid, errno); - return(-1); + printk("Failed to create '%s' - err = %d\n", umid, err); + goto err_rmdir; } umid_setup = 1; @@ -217,13 +252,18 @@ int __init make_umid(int (*printer)(const char *fmt, ...)) create_pid_file(); return 0; + + err_rmdir: + rmdir(tmp); + err: + return err; } static int __init make_umid_init(void) { - make_umid(printk); + make_umid(); - return(0); + return 0; } __initcall(make_umid_init); @@ -232,48 +272,48 @@ int __init umid_file_name(char *name, char *buf, int len) { int n, err; - if(!umid_setup){ - err = make_umid(printk); - if(err) - return err; - } + err = make_umid(); + if(err) + return err; - n = strlen(uml_dir) + strlen(umid) + strlen("/") + strlen(name) + 1; - if(n > len){ + n = snprintf(buf, len, "%s%s/%s", uml_dir, umid, name); + if(n >= len){ printk("umid_file_name : buffer too short\n"); - return(-1); + return -E2BIG; } - sprintf(buf, "%s%s/%s", uml_dir, umid, name); - return(0); + return 0; } -extern int umid_is_random; - -char *get_umid(int only_if_set) +char *get_umid(void) { - if(only_if_set && umid_is_random) - return NULL; return umid; } static int __init set_uml_dir(char *name, int *add) { - if((strlen(name) > 0) && (name[strlen(name) - 1] != '/')){ - uml_dir = malloc(strlen(name) + 2); - if(uml_dir == NULL){ - printf("Failed to malloc uml_dir - error = %d\n", - errno); - uml_dir = name; - /* Return 0 here because do_initcalls doesn't look at - * the return value. - */ - return(0); - } - sprintf(uml_dir, "%s/", name); + if(*name == '\0'){ + printf("uml_dir can't be an empty string\n"); + return 0; } - else uml_dir = name; - return(0); + + if(name[strlen(name) - 1] == '/'){ + uml_dir = name; + return 0; + } + + uml_dir = malloc(strlen(name) + 2); + if(uml_dir == NULL){ + printf("Failed to malloc uml_dir - error = %d\n", errno); + + /* Return 0 here because do_initcalls doesn't look at + * the return value. + */ + return 0; + } + sprintf(uml_dir, "%s/", name); + + return 0; } __uml_setup("uml_dir=", set_uml_dir, @@ -283,10 +323,13 @@ __uml_setup("uml_dir=", set_uml_dir, static void remove_umid_dir(void) { - char dir[strlen(uml_dir) + UMID_LEN + 1]; + char dir[strlen(uml_dir) + UMID_LEN + 1], err; sprintf(dir, "%s%s", uml_dir, umid); - actually_do_remove(dir); + err = actually_do_remove(dir); + if(err) + printf("remove_umid_dir - actually_do_remove failed with " + "err = %d\n", err); } __uml_exitcall(remove_umid_dir); -- 2.43.2