From 177f6eac1f5f80b1ebede7a011b4509355256e9d Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 10 Apr 2022 14:43:50 +0200 Subject: [PATCH 01/56] Makefile: Udpate version to 0.30.6 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index d9ec74e5..ec34bde5 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ MXQ_VERSION_MAJOR = 0 MXQ_VERSION_MINOR = 30 -MXQ_VERSION_PATCH = 5 +MXQ_VERSION_PATCH = 6 MXQ_VERSION_EXTRA = "beta" MXQ_VERSIONDATE = 2022 From 0ea4705ce24660c66b8efbbf2d732652de12415d Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 19:36:45 +0200 Subject: [PATCH 02/56] mxqset: Remove unused variable and argument --- mxqset.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/mxqset.c b/mxqset.c index 18cf375a..c67e4e33 100644 --- a/mxqset.c +++ b/mxqset.c @@ -195,7 +195,7 @@ static const struct argp_option options[] = { static const struct argp argp = { options, parser, NULL, NULL }; -static __attribute__ ((noreturn)) void exit_usage(char *argv0) { +static __attribute__ ((noreturn)) void exit_usage(void) { fprintf(stderr, "usage: %s group GID [group-options]\n" "\n" @@ -210,14 +210,12 @@ static __attribute__ ((noreturn)) void exit_usage(char *argv0) { } int main(int argc, char **argv) { - - char *argv0=argv[0]; struct mx_mysql *mysql = NULL; int groupid; uid_t uid = getuid(); if (argc<3 || strcmp(argv[1],"group") != 0) - exit_usage(argv0); + exit_usage(); groupid=atoi(argv[2]); int sts; @@ -225,7 +223,7 @@ int main(int argc, char **argv) { sts=argp_parse (&argp, argc-3, &argv[3], ARGP_PARSE_ARGV0|ARGP_SILENT, NULL, &opts); if (sts) - exit_usage(argv0); + exit_usage(); assert(mx_mysql_initialize(&mysql) == 0); mx_mysql_option_set_default_file(mysql, MXQ_MYSQL_DEFAULT_FILE); From 8318176cc9801949cc7f4365198fb7eb488d3acf Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 20:42:44 +0200 Subject: [PATCH 03/56] mxqset: Use initializers with designators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not all fields of struct argp_option and struct argp are initialized: mxqset.c:192:5: warning: missing initializer for field ‘group’ of ‘const struct argp_option’ [-Wmissing-field-initializers] {"whitelist", 15, "", 0, NULL}, mxqset.c:196:21: warning: missing initializer for field ‘children’ of ‘const struct argp’ [-Wmissing-field-initializers] static const struct argp argp = { options, parser, NULL, NULL } Use structure initializers with designators, which initialize all remaining fields with their zero values. --- mxqset.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mxqset.c b/mxqset.c index c67e4e33..126471f6 100644 --- a/mxqset.c +++ b/mxqset.c @@ -186,14 +186,14 @@ static error_t parser (int key, char *arg, struct argp_state *state) { } static const struct argp_option options[] = { - {"closed", 10, NULL, 0, NULL}, - {"open", 11, NULL, 0, NULL}, - {"blacklist", 13, "", 0, NULL}, - {"whitelist", 15, "", 0, NULL}, + { .name = "closed", .key = 10 }, + { .name = "open", .key = 11 }, + { .name = "blacklist", .key = 13 }, + { .name = "whitelist", .key = 15 }, {0} }; -static const struct argp argp = { options, parser, NULL, NULL }; +static const struct argp argp = { .options = options, .parser = parser }; static __attribute__ ((noreturn)) void exit_usage(void) { fprintf(stderr, From a67850df01f04267bd618eac260890da0ef02032 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 20:54:13 +0200 Subject: [PATCH 04/56] tree: Add fall through annotations Gcc can warn on implicit fallthoughs. Mark them with a comment which is recognized by gcc. --- mx_util.c | 18 +++++++++--------- mxqsub.c | 2 ++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/mx_util.c b/mx_util.c index 66a51f28..6d72f4a0 100644 --- a/mx_util.c +++ b/mx_util.c @@ -112,17 +112,17 @@ int mx_strtobytes(char *str, unsigned long long int *bytes) case 'T': /* tebi */ t *= 1024; - + // fall through case 'G': /* gibi */ t *= 1024; - + // fall through case 'M': /* mebi */ t *= 1024; - + // fall through case 'k': /* kibi */ case 'K': t *= 1024; - + // fall through case 'B': /* bytes */ end++; break; @@ -180,19 +180,19 @@ int mx_strtoseconds(char *str, unsigned long long int *seconds) case 'y': /* years */ t *= 52; - + // fall through case 'w': /* weeks */ t *= 7; - + // fall through case 'd': /* days */ t *= 24; - + // fall through case 'h': /* hours */ t *= 60; - + // fall through case 'm': /* minutes */ t *= 60; - + // fall through case 's': /* seconds */ end++; break; diff --git a/mxqsub.c b/mxqsub.c index af901d67..52e941dd 100644 --- a/mxqsub.c +++ b/mxqsub.c @@ -902,6 +902,7 @@ int main(int argc, char *argv[]) case 2: mx_log_warning("option --group_priority is deprecated. please use --group-priority instead."); + // fall through case 'P': if (mx_strtou16(optctl.optarg, &arg_group_priority) < 0) { mx_log_crit("--group-priority '%s': %m", optctl.optarg); @@ -931,6 +932,7 @@ int main(int argc, char *argv[]) case 4: mx_log_warning("option '--time' is deprecated. please use '--runtime' or '-t' in future calls."); + // fall through case 't': if (mx_strtou32(optctl.optarg, &arg_time) < 0) { unsigned long long int minutes; From 0052141ee70d2ba6c2dde6435e26593b291b52dd Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 20:57:50 +0200 Subject: [PATCH 05/56] mxqd: Make slots_to_start unsigned Make slots_to_start unsigned to match slots_per_job with which we compare. --- mxqd.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mxqd.c b/mxqd.c index bdb31eaa..11f7efe1 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1401,7 +1401,7 @@ static unsigned long start_job(struct mxq_group_list *glist) return 1; } -static int can_start_job(struct mxq_group_list *group, unsigned long df_scratch, struct mxq_server *server, long slots_to_start) { +static int can_start_job(struct mxq_group_list *group, unsigned long df_scratch, struct mxq_server *server, unsigned long slots_to_start) { /* Can we start a(nother) job from this group */ if (group->jobs_running >= group->group.group_jobs) return 0; @@ -1426,7 +1426,7 @@ static int can_start_job_for_user(struct mxq_user_list *user, unsigned long df_s return 0; } -static unsigned long start_user(struct mxq_user_list *ulist, long slots_to_start, unsigned long df_scratch) +static unsigned long start_user(struct mxq_user_list *ulist, unsigned long slots_to_start, unsigned long df_scratch) { struct mxq_server *server; struct mxq_group_list *glist; @@ -1442,7 +1442,7 @@ static unsigned long start_user(struct mxq_user_list *ulist, long slots_to_start assert(slots_to_start <= server->slots - server->slots_running); - mx_log_debug(" user=%s(%d) slots_to_start=%ld :: trying to start jobs for user.", + mx_log_debug(" user=%s(%d) slots_to_start=%lu :: trying to start jobs for user.", group->user_name, group->user_uid, slots_to_start); for (glist = ulist->groups; glist ; glist = glist->next) { @@ -1450,7 +1450,7 @@ static unsigned long start_user(struct mxq_user_list *ulist, long slots_to_start group = &glist->group; if (can_start_job(glist, df_scratch, server, slots_to_start)) { - mx_log_info(" group=%s(%d):%lu slots_to_start=%ld slots_per_job=%lu :: trying to start job for group.", + mx_log_info(" group=%s(%d):%lu slots_to_start=%lu slots_per_job=%lu :: trying to start job for group.", group->user_name, group->user_uid, group->group_id, slots_to_start, glist->slots_per_job); if (start_job(glist)) { int slots_started = glist->slots_per_job; From ad98991bc9871f2e36859e832ad22486eb5a6adf Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:00:24 +0200 Subject: [PATCH 06/56] mx_proc: getline returns signed size type --- mx_proc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mx_proc.c b/mx_proc.c index 48e590cc..4ffbaa70 100644 --- a/mx_proc.c +++ b/mx_proc.c @@ -18,7 +18,7 @@ static long long int get_rss_anon(pid_t pid) { _mx_cleanup_free_ char *buf = NULL; size_t n = 0; while(1) { - size_t len = getline(&buf, &n, file); + ssize_t len = getline(&buf, &n, file); if (len == -1) break; if (strncmp(buf, "RssAnon:", 8) == 0) { From de3553c881946c0c080dd97ad7c6bff4223144e7 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:03:40 +0200 Subject: [PATCH 07/56] mxq_job: Remove unused parameter from mxq_unload_job_from_server --- mxq_job.c | 2 +- mxq_job.h | 2 +- mxqd.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/mxq_job.c b/mxq_job.c index 08217fcb..0a02f3d9 100644 --- a/mxq_job.c +++ b/mxq_job.c @@ -803,7 +803,7 @@ int mxq_load_jobs_running_on_server(struct mx_mysql *mysql, struct mxq_job **job return res; } -int mxq_unload_job_from_server(struct mx_mysql *mysql, struct mxq_daemon *daemon, uint64_t job_id) { +int mxq_unload_job_from_server(struct mx_mysql *mysql, uint64_t job_id) { /* set a job from LOADED back to INQ. This needs to reset what * mxq_assign_job_from_group_to_daemon() and mxq_set_job_status_loaded_on_server() diff --git a/mxq_job.h b/mxq_job.h index f6a9c314..15699b65 100644 --- a/mxq_job.h +++ b/mxq_job.h @@ -93,5 +93,5 @@ int mxq_set_job_status_unknown(struct mx_mysql *mysql, struct mxq_job *job); int mxq_job_set_tmpfilenames(struct mxq_group *g, struct mxq_job *j); int mxq_load_job_from_group_for_daemon(struct mx_mysql *mysql, struct mxq_job *mxqjob, uint64_t group_id, struct mxq_daemon *daemon,unsigned long slots_per_job); int mxq_load_jobs_running_on_server(struct mx_mysql *mysql, struct mxq_job **jobs_result, struct mxq_daemon *daemon); -int mxq_unload_job_from_server(struct mx_mysql *mysql, struct mxq_daemon *daemon, uint64_t job_id); +int mxq_unload_job_from_server(struct mx_mysql *mysql, uint64_t job_id); #endif diff --git a/mxqd.c b/mxqd.c index 11f7efe1..008a447f 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1323,12 +1323,12 @@ static unsigned long start_job(struct mxq_group_list *glist) mx_mysql_connect_forever(&(server->mysql)); if (pid < 0) { mx_log_err("fork: %m"); - mxq_unload_job_from_server(server->mysql, daemon, job->job_id); + mxq_unload_job_from_server(server->mysql, job->job_id); return(0); } waitpid(pid, &status, 0); if (status) { - mxq_unload_job_from_server(server->mysql, daemon, job->job_id); + mxq_unload_job_from_server(server->mysql, job->job_id); return 0; } } @@ -1343,7 +1343,7 @@ static unsigned long start_job(struct mxq_group_list *glist) if (pid < 0) { mx_log_err("fork: %m"); cpuset_clear_running(&job->host_cpu_set,&server->cpu_set_available); - mxq_unload_job_from_server(server->mysql, daemon, job->job_id); + mxq_unload_job_from_server(server->mysql, job->job_id); return 0; } else if (pid == 0) { job->host_pid = getpid(); From 9328af6c5f83954fd389ab6f20486c44d08d3308 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:09:24 +0200 Subject: [PATCH 08/56] mxqsub: Remove unused parameter flags from mxq_submit_task --- mxqsub.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mxqsub.c b/mxqsub.c index 52e941dd..802d13b2 100644 --- a/mxqsub.c +++ b/mxqsub.c @@ -602,7 +602,7 @@ static int get_active_groups_for_user(struct mx_mysql *mysql, char *username) return count; } -static int mxq_submit_task(struct mx_mysql *mysql, struct mxq_job *j, int flags, uint64_t group_id) +static int mxq_submit_task(struct mx_mysql *mysql, struct mxq_job *j, uint64_t group_id) { int res; struct mxq_group *g; @@ -724,8 +724,6 @@ int main(int argc, char *argv[]) _mx_cleanup_free_ char *whitelist = NULL; _mx_cleanup_free_ char *tags = NULL; - int flags = 0; - struct mxq_job job; struct mxq_group group; @@ -1187,7 +1185,7 @@ int main(int argc, char *argv[]) mx_log_info("MySQL: Connection to database established."); - res = mxq_submit_task(mysql, &job, flags, arg_groupid); + res = mxq_submit_task(mysql, &job, arg_groupid); mx_mysql_finish(&mysql); From 0c35a52f280e1de89b8a41cd484e37143a0b10f5 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:14:32 +0200 Subject: [PATCH 09/56] mx_myqsl: Don't compare signed int and unsigned long --- mx_mysql.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/mx_mysql.c b/mx_mysql.c index ac066671..aff7bc02 100644 --- a/mx_mysql.c +++ b/mx_mysql.c @@ -699,11 +699,9 @@ static int _mx_mysql_bind_string(struct mx_mysql_bind *b, unsigned int index, ch static int _mx_mysql_bind_validate(struct mx_mysql_bind *b) { - int i; - mx_assert_return_minus_errno(b, EINVAL); - for (i=0; i < b->count; i++) { + for (unsigned long i=0; i < b->count; i++) { if (!(b->data[i].flags)) { return -(errno=EBADSLT); } @@ -1015,7 +1013,6 @@ int mx_mysql_statement_fetch(struct mx_mysql_stmt *stmt) { struct mx_mysql_bind *r; int res; - int col; char *str; int no_error = 1; @@ -1044,7 +1041,7 @@ int mx_mysql_statement_fetch(struct mx_mysql_stmt *stmt) } r = &stmt->result; - for (col = 0; col < r->count; col++) { + for (unsigned long col = 0; col < r->count; col++) { if (r->bind[col].buffer_type == MYSQL_TYPE_STRING) { str = mx_calloc_forever(r->data[col].length + 1, sizeof(*str)); @@ -1063,7 +1060,7 @@ int mx_mysql_statement_fetch(struct mx_mysql_stmt *stmt) if (!(r->data[col].is_error)) continue; - mx_log_debug("WARNING: result data returned in column with index %d was truncated. query was:", col); + mx_log_debug("WARNING: result data returned in column with index %lu was truncated. query was:", col); mx_log_debug(" \\ %s", stmt->statement); no_error = 0; } @@ -1165,7 +1162,6 @@ static int _mx_mysql_do_statement(struct mx_mysql *mysql, char *query, struct mx struct mx_mysql_stmt *stmt = NULL; unsigned long long num_rows = 0; int res; - int cnt = 0; char *tmpdata; assert(mysql); @@ -1196,7 +1192,7 @@ static int _mx_mysql_do_statement(struct mx_mysql *mysql, char *query, struct mx if (result && result->count && num_rows) { tmpdata = mx_calloc_forever(num_rows, size); - for (cnt = 0; cnt < num_rows; cnt++) { + for (unsigned long cnt = 0; cnt < num_rows; cnt++) { res = mx_mysql_statement_fetch(stmt); if (res < 0) { mx_log_err("mx_mysql_statement_fetch(): %m"); From 98e4d2cf54b250558a041f1adef9461ee550e7e9 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:22:26 +0200 Subject: [PATCH 10/56] keywordset: Match type of local variable to argument --- keywordset.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keywordset.c b/keywordset.c index 1ce14df3..e9d6da95 100644 --- a/keywordset.c +++ b/keywordset.c @@ -15,7 +15,7 @@ struct keywordset { static int find_name(struct keywordset *kws, char *name, size_t len) { int i; - int j; + size_t j; for ( i = 0; i < kws->used ; i++ ) { j = 0; while(1) { From 5e3cc57c07f926a9beb5acd4d1d0c7a95e575540 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:39:46 +0200 Subject: [PATCH 11/56] mxqd_control: Explicitly convert pid_t to unsigned --- mxqd_control.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mxqd_control.c b/mxqd_control.c index 47903673..d51ffebc 100644 --- a/mxqd_control.c +++ b/mxqd_control.c @@ -206,7 +206,7 @@ struct mxq_job_list *server_get_job_list_by_pid(struct mxq_server *server, pid_t for (glist = ulist->groups; glist; glist = glist->next) { for (jlist = glist->jobs; jlist; jlist = jlist->next) { job = &jlist->job; - if (job->host_pid == pid) + if (job->host_pid == (unsigned)pid) return jlist; } } From bbd0b5c83094c01305160b5fd365995eb4e1eedf Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:40:24 +0200 Subject: [PATCH 12/56] parser: Annotate unused arguments --- parser.y | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/parser.y b/parser.y index 835486cc..503d0df6 100644 --- a/parser.y +++ b/parser.y @@ -53,6 +53,7 @@ bool: '(' bool ')' { $$ = $2; }; #include "xmalloc.h" int yylex (YYSTYPE *lvalp, YYLTYPE *llocp, struct parser_context *ctx) { + (void)llocp; int c = ctx->input[ctx->pos]; while (c == ' ' || c == '\t') @@ -88,6 +89,9 @@ int yylex (YYSTYPE *lvalp, YYLTYPE *llocp, struct parser_context *ctx) { #include void yyerror (YYLTYPE *locp, struct parser_context *ctx, char const *s) { + (void)locp; + (void)ctx; + (void)s; } From e6edbd5b8020d62c9fd2b6a544fed800f2e99418 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 10:56:41 +0200 Subject: [PATCH 13/56] test_*: Remove unused parameters from main --- test_mx_log.c | 2 +- test_mx_util.c | 2 +- test_mxqd_control.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test_mx_log.c b/test_mx_log.c index 5d1fb05f..8233694d 100644 --- a/test_mx_log.c +++ b/test_mx_log.c @@ -93,7 +93,7 @@ static void test_mx_log_level_syslog_to_mxlog(void) assert(mx_log_level_syslog_to_mxlog(LOG_DEBUG) == MX_LOG_DEBUG); } -int main(int argc, char *argv[]) +int main(void) { test_mx_log_level_syslog_to_mxlog(); test_mx_log_level_mxlog_to_syslog(); diff --git a/test_mx_util.c b/test_mx_util.c index 20b58c81..92959809 100644 --- a/test_mx_util.c +++ b/test_mx_util.c @@ -556,7 +556,7 @@ static void test_mx_call_external(void) { } -int main(int argc, char *argv[]) +int main(void) { test_mx_strskipwhitespaces(); test_mx_strtoul(); diff --git a/test_mxqd_control.c b/test_mxqd_control.c index c8ba25b3..00f65968 100644 --- a/test_mxqd_control.c +++ b/test_mxqd_control.c @@ -29,7 +29,7 @@ static void test_mxqd_control(void) assert(1); } -int main(int argc, char *argv[]) +int main(void) { test_mxqd_control(); return 0; From 421dc0c842b3a4e5e037a9ee53813ce2ff1f15ab Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 19:18:30 +0200 Subject: [PATCH 14/56] mx_util: Simplify signature of mx_vasprintf_forever mx_vasprintf_forever and mx_asprintf_forever can't fail, so don't return an (always zero) status but the allocated string instead. --- mx_proc.c | 6 ++---- mx_util.c | 21 ++++++++++----------- mx_util.h | 4 ++-- mxq_job.c | 7 +++---- mxqd.c | 36 ++++++++++++++---------------------- 5 files changed, 31 insertions(+), 43 deletions(-) diff --git a/mx_proc.c b/mx_proc.c index 4ffbaa70..72330df4 100644 --- a/mx_proc.c +++ b/mx_proc.c @@ -10,8 +10,7 @@ #include "mx_proc.h" static long long int get_rss_anon(pid_t pid) { - _mx_cleanup_free_ char *fname; - mx_asprintf_forever(&fname, "/proc/%d/status", pid); + _mx_cleanup_free_ char *fname = mx_asprintf_forever("/proc/%d/status", pid); _mx_cleanup_fclose_ FILE *file = fopen(fname, "r"); if (file == NULL) return -errno; @@ -131,7 +130,6 @@ int mx_proc_pid_stat(struct mx_proc_pid_stat **pps, pid_t pid) int mx_proc_pid_stat_read(struct mx_proc_pid_stat *pps, char *fmt, ...) { - _mx_cleanup_free_ char *fname = NULL; _mx_cleanup_free_ char *line = NULL; va_list ap; int res; @@ -139,7 +137,7 @@ int mx_proc_pid_stat_read(struct mx_proc_pid_stat *pps, char *fmt, ...) assert(pps); va_start(ap, fmt); - mx_vasprintf_forever(&fname, fmt, ap); + _mx_cleanup_free_ char *fname = fname = mx_vasprintf_forever(fmt, ap); va_end(ap); res = mx_read_first_line_from_file(fname, &line); diff --git a/mx_util.c b/mx_util.c index 6d72f4a0..2d41bf59 100644 --- a/mx_util.c +++ b/mx_util.c @@ -580,28 +580,28 @@ char *mx_strdup_forever(char *str) return dup; } -int mx_vasprintf_forever(char **strp, const char *fmt, va_list ap) +char *mx_vasprintf_forever(const char *fmt, va_list ap) { int len; + char *strp; do { - len = vasprintf(strp, fmt, ap); + len = vasprintf(&strp, fmt, ap); } while (len < 0); - return len; + return strp; } -int mx_asprintf_forever(char **strp, const char *fmt, ...) +char *mx_asprintf_forever(const char *fmt, ...) { va_list ap; - int len; va_start(ap, fmt); - len = mx_vasprintf_forever(strp, fmt, ap); + char *strp = mx_vasprintf_forever(fmt, ap); va_end(ap); - return len; + return strp; } char *mx_hostname(void) @@ -721,11 +721,10 @@ int mx_setenvf_forever(const char *name, char *fmt, ...) assert(fmt); va_list ap; - char *value = NULL; int res; va_start(ap, fmt); - mx_vasprintf_forever(&value, fmt, ap); + char *value = mx_vasprintf_forever(fmt, ap); va_end(ap); res = mx_setenv_forever(name, value); @@ -1217,9 +1216,9 @@ char *mx_cpuset_to_str(cpu_set_t* cpuset_ptr) } cpu_high=cpu-1; if (cpu_low==cpu_high) { - mx_asprintf_forever(&str,"%d",cpu_low); + str = mx_asprintf_forever("%d", cpu_low); } else { - mx_asprintf_forever(&str,"%d-%d",cpu_low,cpu_high); + str = mx_asprintf_forever("%d-%d", cpu_low, cpu_high); } res=mx_strvec_push_str(&strvec,str); if (!res) { diff --git a/mx_util.h b/mx_util.h index e7971261..92747f9a 100644 --- a/mx_util.h +++ b/mx_util.h @@ -125,8 +125,8 @@ int mx_strtoi64(char *str, int64_t *to); void *mx_malloc_forever(size_t size); char *mx_strdup_forever(char *str); -int mx_vasprintf_forever(char **strp, const char *fmt, va_list ap); -int mx_asprintf_forever(char **strp, const char *fmt, ...) __attribute__ ((format(printf, 2, 3))); +char *mx_vasprintf_forever(const char *fmt, va_list ap); +char *mx_asprintf_forever(const char *fmt, ...) __attribute__ ((format(printf, 1, 2))); char *mx_hostname(void); char *mx_dirname(char *path); diff --git a/mxq_job.c b/mxq_job.c index 0a02f3d9..ba8a606a 100644 --- a/mxq_job.c +++ b/mxq_job.c @@ -426,7 +426,6 @@ int mxq_unassign_jobs_of_server(struct mx_mysql *mysql, struct mxq_daemon *daemo static int mxq_set_job_status_loaded_on_server(struct mx_mysql *mysql, struct mxq_job *job) { struct mx_mysql_bind param = {0}; - char *host_id; int res; int idx; @@ -435,7 +434,7 @@ static int mxq_set_job_status_loaded_on_server(struct mx_mysql *mysql, struct mx assert(job->job_id); assert(job->daemon_id); - mx_asprintf_forever(&host_id, "%u", job->daemon_id); + char *host_id = mx_asprintf_forever("%u", job->daemon_id); char *query = "UPDATE" @@ -655,7 +654,7 @@ int mxq_job_set_tmpfilenames(struct mxq_group *g, struct mxq_job *j) dir = mx_dirname_forever(j->job_stdout); - mx_asprintf_forever(&j->tmp_stdout, "%s/mxq.%u.%lu.%lu.%s.%s.%d.stdout.tmp", + j->tmp_stdout = mx_asprintf_forever("%s/mxq.%u.%lu.%lu.%s.%s.%d.stdout.tmp", dir, g->user_uid, g->group_id, j->job_id, j->host_hostname, j->daemon_name, j->host_pid); } @@ -669,7 +668,7 @@ int mxq_job_set_tmpfilenames(struct mxq_group *g, struct mxq_job *j) } dir = mx_dirname_forever(j->job_stderr); - mx_asprintf_forever(&j->tmp_stderr, "%s/mxq.%u.%lu.%lu.%s.%s.%d.stderr.tmp", + j->tmp_stderr = mx_asprintf_forever("%s/mxq.%u.%lu.%lu.%s.%s.%d.stderr.tmp", dir, g->user_uid, g->group_id, j->job_id, j->host_hostname, j->daemon_name, j->host_pid); } diff --git a/mxqd.c b/mxqd.c index 008a447f..379489bc 100644 --- a/mxqd.c +++ b/mxqd.c @@ -666,7 +666,7 @@ static int server_init(struct mxq_server *server, int argc, char *argv[]) return -EX_UNAVAILABLE; } - mx_asprintf_forever(&server->finished_jobsdir,"%s/%s",MXQ_FINISHED_JOBSDIR,server->daemon_name); + server->finished_jobsdir = mx_asprintf_forever("%s/%s", MXQ_FINISHED_JOBSDIR, server->daemon_name); res=mx_mkdir_p(server->finished_jobsdir,0700); if (res<0) { mx_log_err("MAIN: mkdir %s failed: %m. Exiting.",MXQ_FINISHED_JOBSDIR); @@ -746,7 +746,7 @@ static int server_init(struct mxq_server *server, int argc, char *argv[]) server->starttime = pps->starttime; mx_proc_pid_stat_free_content(pps); - mx_asprintf_forever(&server->host_id, "%s-%llx-%x", server->boot_id, server->starttime, getpid()); + server->host_id = mx_asprintf_forever("%s-%llx-%x", server->boot_id, server->starttime, getpid()); mx_setenv_forever("MXQ_HOSTID", server->host_id); server->slots = arg_threads_total; @@ -895,17 +895,14 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) if (group->job_tmpdir_size == 0) { mx_setenv_forever("TMPDIR", server->initial_tmpdir); } else { - char *mxq_job_tmpdir; - mx_asprintf_forever(&mxq_job_tmpdir, "%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job->job_id); + char *mxq_job_tmpdir = mx_asprintf_forever("%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job->job_id); mx_setenv_forever("MXQ_JOB_TMPDIR", mxq_job_tmpdir); mx_setenv_forever("TMPDIR", mxq_job_tmpdir); free(mxq_job_tmpdir); } if (group->job_gpu) { - char *pid; - char *uid; - mx_asprintf_forever(&pid, "%d", job->host_pid); - mx_asprintf_forever(&uid, "%u", group->user_uid); + char *pid = mx_asprintf_forever("%d", job->host_pid); + char *uid = mx_asprintf_forever("%u", group->user_uid); char *gpu_uuid = mx_call_external(gpu_setup_script, "job-init", pid, uid, NULL); if (!gpu_uuid) { mx_log_err("gpu-setup job-init: %m"); @@ -1231,8 +1228,8 @@ static int reaper_process(struct mxq_server *server,struct mxq_group_list *glist return(res); } - mx_asprintf_forever(&finished_job_filename, "%s/%lu.stat", server->finished_jobsdir, job->job_id); - mx_asprintf_forever(&finished_job_tmpfilename, "%s.tmp", finished_job_filename); + finished_job_filename = mx_asprintf_forever("%s/%lu.stat", server->finished_jobsdir, job->job_id); + finished_job_tmpfilename = mx_asprintf_forever("%s.tmp", finished_job_filename); out=fopen(finished_job_tmpfilename,"w"); if (!out) { @@ -1312,9 +1309,9 @@ static unsigned long start_job(struct mxq_group_list *glist) char *envp[4]; argv[0] = create_job_tmpdir_script, argv[1] = NULL; - mx_asprintf_forever(&envp[0], "MXQ_JOBID=%lu", job->job_id); - mx_asprintf_forever(&envp[1], "MXQ_SIZE=%u", group->job_tmpdir_size); - mx_asprintf_forever(&envp[2], "MXQ_UID=%d", group->user_uid); + envp[0] = mx_asprintf_forever("MXQ_JOBID=%lu", job->job_id); + envp[1] = mx_asprintf_forever("MXQ_SIZE=%u", group->job_tmpdir_size); + envp[2] = mx_asprintf_forever("MXQ_UID=%d", group->user_uid); envp[3] = NULL; execve(create_job_tmpdir_script,argv,envp); mx_log_fatal("exec %s : %m",create_job_tmpdir_script); @@ -2022,9 +2019,7 @@ static void rename_outfiles(struct mxq_server *server, struct mxq_group *group, } static char *job_tmpdir_path(unsigned long job_id) { - char *pathname; - mx_asprintf_forever(&pathname, "%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job_id); - return pathname; + return mx_asprintf_forever("%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job_id); } static int unmount_and_remove(char *pathname) { @@ -2050,8 +2045,7 @@ static void unmount_job_tmpdir(unsigned long job_id) { static void release_gpu(struct mxq_server *server, struct mxq_group *group, struct mxq_job *job) { if (group->job_gpu) { - char *pid; - mx_asprintf_forever(&pid, "%d", job->host_pid); + char *pid = mx_asprintf_forever("%d", job->host_pid); char *gpu_uuid = mx_call_external(gpu_setup_script, "job-release", pid, NULL); free(pid); if (!gpu_uuid) { @@ -2111,9 +2105,7 @@ static int job_is_lost(struct mxq_server *server,struct mxq_group *group, struct static char *fspool_get_filename (struct mxq_server *server,long unsigned int job_id) { - char *fspool_filename; - mx_asprintf_forever(&fspool_filename,"%s/%lu.stat",server->finished_jobsdir,job_id); - return fspool_filename; + return mx_asprintf_forever("%s/%lu.stat", server->finished_jobsdir, job_id); } static int fspool_process_file(struct mxq_server *server,char *filename, uint64_t job_id) { @@ -2244,7 +2236,7 @@ static int fspool_scan(struct mxq_server *server) { } for (i=0;ifinished_jobsdir,namelist[i]->d_name); + filename = mx_asprintf_forever("%s/%s", server->finished_jobsdir, namelist[i]->d_name); if (fspool_is_valid_name_parse(namelist[i]->d_name,&job_id)) { res=fspool_process_file(server,filename,job_id); if (res>0) { From de9849c02c7e65e2a0a9fad259eb629e80cd83c2 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 18:03:36 +0200 Subject: [PATCH 15/56] mxqd: Remove two functions Remove two functions which have degenerated into a single source line and have one caller only. --- mxqd.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/mxqd.c b/mxqd.c index 379489bc..085c4885 100644 --- a/mxqd.c +++ b/mxqd.c @@ -2018,10 +2018,6 @@ static void rename_outfiles(struct mxq_server *server, struct mxq_group *group, } } -static char *job_tmpdir_path(unsigned long job_id) { - return mx_asprintf_forever("%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job_id); -} - static int unmount_and_remove(char *pathname) { int res; res = rmdir(pathname); @@ -2035,8 +2031,7 @@ static int unmount_and_remove(char *pathname) { } static void unmount_job_tmpdir(unsigned long job_id) { - char *pathname; - pathname=job_tmpdir_path(job_id); + char *pathname = mx_asprintf_forever("%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job_id); if (unmount_and_remove(pathname)) { mx_log_warning("failed to unmount/remove stale job tmpdir %s: %m", pathname); } @@ -2103,11 +2098,6 @@ static int job_is_lost(struct mxq_server *server,struct mxq_group *group, struct return cnt; } -static char *fspool_get_filename (struct mxq_server *server,long unsigned int job_id) -{ - return mx_asprintf_forever("%s/%lu.stat", server->finished_jobsdir, job_id); -} - static int fspool_process_file(struct mxq_server *server,char *filename, uint64_t job_id) { FILE *in; int res; @@ -2269,8 +2259,7 @@ static int file_exists(char *name) { } static int fspool_file_exists(struct mxq_server *server,uint64_t job_id) { - _mx_cleanup_free_ char *fspool_filename=NULL; - fspool_filename=fspool_get_filename(server,job_id); + _mx_cleanup_free_ char *fspool_filename = mx_asprintf_forever("%s/%lu.stat", server->finished_jobsdir, job_id); return file_exists(fspool_filename); } From fe3c128ab74c7bfbb426405579373ec5116e29f4 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 10:54:57 +0200 Subject: [PATCH 16/56] mx_util: Rename mx_call_external* to mx_pipe_external* --- mx_util.c | 6 +++--- mx_util.h | 2 +- mxqd.c | 8 ++++---- test_mx_util.c | 18 +++++++++--------- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/mx_util.c b/mx_util.c index 2d41bf59..3e84cdb0 100644 --- a/mx_util.c +++ b/mx_util.c @@ -1330,7 +1330,7 @@ static ssize_t readall(int fd, char *buf, size_t buflen) { return len; } -static char *mx_call_external_v(char *helper, char **argv) { +static char *mx_pipe_external_v(char *helper, char **argv) { int pipefd[2]; int err; char buf[2048]; @@ -1385,7 +1385,7 @@ static char *mx_call_external_v(char *helper, char **argv) { return NULL; } -char *mx_call_external(char *cmd, ...) { +char *mx_pipe_external(char *cmd, ...) { int len = 0; va_list ap, ap2; char **argv; @@ -1404,5 +1404,5 @@ char *mx_call_external(char *cmd, ...) { argv[i++] = va_arg(ap2, char *); } while (ijob_gpu) { char *pid = mx_asprintf_forever("%d", job->host_pid); char *uid = mx_asprintf_forever("%u", group->user_uid); - char *gpu_uuid = mx_call_external(gpu_setup_script, "job-init", pid, uid, NULL); + char *gpu_uuid = mx_pipe_external(gpu_setup_script, "job-init", pid, uid, NULL); if (!gpu_uuid) { mx_log_err("gpu-setup job-init: %m"); exit(1); @@ -2041,7 +2041,7 @@ static void unmount_job_tmpdir(unsigned long job_id) { static void release_gpu(struct mxq_server *server, struct mxq_group *group, struct mxq_job *job) { if (group->job_gpu) { char *pid = mx_asprintf_forever("%d", job->host_pid); - char *gpu_uuid = mx_call_external(gpu_setup_script, "job-release", pid, NULL); + char *gpu_uuid = mx_pipe_external(gpu_setup_script, "job-release", pid, NULL); free(pid); if (!gpu_uuid) { mx_log_err("gpu-setup job-release: %m"); diff --git a/test_mx_util.c b/test_mx_util.c index 92959809..1f81b69e 100644 --- a/test_mx_util.c +++ b/test_mx_util.c @@ -522,36 +522,36 @@ static void test_mx_df(void) { assert(mx_df("/") > 0); } -static void test_mx_call_external(void) { +static void test_mx_pipe_external(void) { char *line; errno = 999; - line = mx_call_external("/usr/bin/echo", "123", NULL); + line = mx_pipe_external("/usr/bin/echo", "123", NULL); assert(line && strcmp(line, "123") == 0); free(line); - line = mx_call_external("/usr/bin/echo", "-n", "123", NULL); + line = mx_pipe_external("/usr/bin/echo", "-n", "123", NULL); assert(line && strcmp(line, "123") == 0); free(line); - line = mx_call_external("/usr/bin/echo", "-ne", "123\n456\n\n", NULL); + line = mx_pipe_external("/usr/bin/echo", "-ne", "123\n456\n\n", NULL); assert(line && strcmp(line, "123\n456\n") == 0); free(line); - line = mx_call_external("/usr/bin/true", NULL); + line = mx_pipe_external("/usr/bin/true", NULL); assert(line && strcmp(line, "") == 0); free(line); assert(errno == 999); - line = mx_call_external("/usr/bin/false", NULL); + line = mx_pipe_external("/usr/bin/false", NULL); assert(line == NULL && errno==EPROTO); - line = mx_call_external("/usr/bin/cat", "/usr/bin/bash", NULL); + line = mx_pipe_external("/usr/bin/cat", "/usr/bin/bash", NULL); assert(line == NULL && errno==EPROTO); - line = mx_call_external("/usr/bin/yes", NULL); + line = mx_pipe_external("/usr/bin/yes", NULL); assert(line == NULL && errno==EPROTO); } @@ -576,6 +576,6 @@ int main(void) test_mx_cpuset(); test_listsort(); test_mx_df(); - test_mx_call_external(); + test_mx_pipe_external(); return 0; } From 55b4c2defd6c07aa5b6938f0dfbf05dab9759dee Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 11:38:59 +0200 Subject: [PATCH 17/56] mx_util: Rework mx_pipe_external_v In child, call _exit() instead of exit() after failed exec() to avoid library destructors being called. Nitpicks: Change pid to pid_t, don't let the child check for fork error. --- mx_util.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/mx_util.c b/mx_util.c index 3e84cdb0..77c476db 100644 --- a/mx_util.c +++ b/mx_util.c @@ -1338,20 +1338,20 @@ static char *mx_pipe_external_v(char *helper, char **argv) { if (pipe(pipefd) < 0) { return NULL; } - int pid = fork(); - if (pid < 0) { - err = errno; - close(pipefd[0]); - close(pipefd[1]); - errno = err; - return NULL; - } + pid_t pid = fork(); if (pid == 0) { close(pipefd[0]); dup2(pipefd[1], 1); execv(helper, argv); mx_log_err("%s: %m", helper); - exit(1); + _exit(1); + } + if (pid == -1) { + err = errno; + close(pipefd[0]); + close(pipefd[1]); + errno = err; + return NULL; } close(pipefd[1]); ssize_t len = readall(pipefd[0], buf, sizeof(buf)); From 97057c404af4c53fd1afbcbf89368a90c8effc10 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 20:52:17 +0200 Subject: [PATCH 18/56] mx_util: Export mx_pipe_external_v --- mx_util.c | 2 +- mx_util.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/mx_util.c b/mx_util.c index 77c476db..51f262f8 100644 --- a/mx_util.c +++ b/mx_util.c @@ -1330,7 +1330,7 @@ static ssize_t readall(int fd, char *buf, size_t buflen) { return len; } -static char *mx_pipe_external_v(char *helper, char **argv) { +char *mx_pipe_external_v(char *helper, char **argv) { int pipefd[2]; int err; char buf[2048]; diff --git a/mx_util.h b/mx_util.h index 8beb8d22..071f00d8 100644 --- a/mx_util.h +++ b/mx_util.h @@ -180,6 +180,7 @@ void _mx_sort_linked_list(void **list, int (*cmp)(void *o1,void *o2), void ** (* unsigned long mx_df(const char *path); time_t mx_clock_boottime(void); +char *mx_pipe_external_v(char *args, char **argv); char *mx_pipe_external(char *args, ...); #endif From 6448ff25db43b65b4da3c643575a0772de79eb2c Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 20:38:21 +0200 Subject: [PATCH 19/56] mxqd: Prefer mx_pipe_external_v over mx_pipe_external Avoid the variadic api, because its slower. Also, the variadic funtions is more prone to errors, because its easy to forget the last argument which must be NULL. --- mxqd.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/mxqd.c b/mxqd.c index 2cc42415..8c37c0e8 100644 --- a/mxqd.c +++ b/mxqd.c @@ -323,7 +323,8 @@ static int cpuset_init(struct mxq_server *server) } static void read_hostconfig_retry(struct keywordset *kws) { - char *line = mx_pipe_external("/usr/sbin/hostconfig", NULL); + char *argv[] = { "/usr/sbin/hostconfig", NULL }; + char *line = mx_pipe_external_v("/usr/sbin/hostconfig", argv); if (!line) { mx_log_err("hostconfig: %m"); exit(1); @@ -335,7 +336,8 @@ static void read_hostconfig_retry(struct keywordset *kws) { static char gpu_setup_script[] = LIBEXECDIR "/mxq/gpu-setup"; static int get_gpus(void) { - char *line = mx_pipe_external(gpu_setup_script, "init", NULL); + char *argv[] = { gpu_setup_script, "init", NULL }; + char *line = mx_pipe_external_v(gpu_setup_script, argv); if (!line) { mx_log_err("gpu-setup init: %m"); exit(1); @@ -901,17 +903,22 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) free(mxq_job_tmpdir); } if (group->job_gpu) { - char *pid = mx_asprintf_forever("%d", job->host_pid); - char *uid = mx_asprintf_forever("%u", group->user_uid); - char *gpu_uuid = mx_pipe_external(gpu_setup_script, "job-init", pid, uid, NULL); + char *argv[] = { + gpu_setup_script, + "job-init", + mx_asprintf_forever("%d", job->host_pid), + mx_asprintf_forever("%u", group->user_uid), + NULL + }; + char *gpu_uuid = mx_pipe_external_v(gpu_setup_script, argv); if (!gpu_uuid) { mx_log_err("gpu-setup job-init: %m"); exit(1); } mx_setenv_forever("CUDA_VISIBLE_DEVICES", gpu_uuid); free(gpu_uuid); - free(pid); - free(uid); + free(argv[2]); + free(argv[3]); } fh = open("/proc/self/loginuid", O_WRONLY|O_TRUNC); @@ -2040,9 +2047,14 @@ static void unmount_job_tmpdir(unsigned long job_id) { static void release_gpu(struct mxq_server *server, struct mxq_group *group, struct mxq_job *job) { if (group->job_gpu) { - char *pid = mx_asprintf_forever("%d", job->host_pid); - char *gpu_uuid = mx_pipe_external(gpu_setup_script, "job-release", pid, NULL); - free(pid); + char *argv[] = { + gpu_setup_script, + "job-release", + mx_asprintf_forever("%d", job->host_pid), + NULL + }; + char *gpu_uuid = mx_pipe_external_v(gpu_setup_script, argv); + free(argv[2]); if (!gpu_uuid) { mx_log_err("gpu-setup job-release: %m"); exit(1); From 66653d00e3b0a99445f7506f94281b203b84a889 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Tue, 3 May 2022 11:40:44 +0200 Subject: [PATCH 20/56] test_mx_util: Prefer mx_pipe_external_v over mx_pipe_external_v --- test_mx_util.c | 58 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/test_mx_util.c b/test_mx_util.c index 1f81b69e..6042cbf1 100644 --- a/test_mx_util.c +++ b/test_mx_util.c @@ -527,33 +527,53 @@ static void test_mx_pipe_external(void) { errno = 999; - line = mx_pipe_external("/usr/bin/echo", "123", NULL); - assert(line && strcmp(line, "123") == 0); - free(line); + { + char *argv[] = { "/usr/bin/echo", "123", NULL }; + line = mx_pipe_external_v("/usr/bin/echo", argv); + assert(line && strcmp(line, "123") == 0); + free(line); + } - line = mx_pipe_external("/usr/bin/echo", "-n", "123", NULL); - assert(line && strcmp(line, "123") == 0); - free(line); + { + char *argv[] = { "/usr/bin/echo", "-n", "123", NULL }; + line = mx_pipe_external_v("/usr/bin/echo", argv); + assert(line && strcmp(line, "123") == 0); + free(line); + } - line = mx_pipe_external("/usr/bin/echo", "-ne", "123\n456\n\n", NULL); - assert(line && strcmp(line, "123\n456\n") == 0); - free(line); + { + char *argv[] = { "/usr/bin/echo", "-ne", "123\n456\n\n", NULL }; + line = mx_pipe_external_v("/usr/bin/echo", argv); + assert(line && strcmp(line, "123\n456\n") == 0); + free(line); + } - line = mx_pipe_external("/usr/bin/true", NULL); - assert(line && strcmp(line, "") == 0); - free(line); + { + char *argv[] = { "/usr/bin/true", NULL }; + line = mx_pipe_external_v("/usr/bin/true", argv); + assert(line && strcmp(line, "") == 0); + free(line); + } assert(errno == 999); - line = mx_pipe_external("/usr/bin/false", NULL); - assert(line == NULL && errno==EPROTO); - - line = mx_pipe_external("/usr/bin/cat", "/usr/bin/bash", NULL); - assert(line == NULL && errno==EPROTO); + { + char *argv[] = { "/usr/bin/false", NULL }; + line = mx_pipe_external_v("/usr/bin/false", argv); + assert(line == NULL && errno==EPROTO); + } - line = mx_pipe_external("/usr/bin/yes", NULL); - assert(line == NULL && errno==EPROTO); + { + char *argv[] = { "/usr/bin/cat", "/usr/bin/bash", NULL }; + line = mx_pipe_external_v("/usr/bin/cat", argv); + assert(line == NULL && errno==EPROTO); + } + { + char *argv[] = { "/usr/bin/yes", NULL }; + line = mx_pipe_external_v("/usr/bin/yes", argv); + assert(line == NULL && errno==EPROTO); + } } int main(void) From 577df460b48efb4d880a05156c524e6a4cebdf84 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 16:48:16 +0200 Subject: [PATCH 21/56] mx_util: Remove mx_pipe_external Remove now unused variadic version of mx_pipe_external, mx_pipe_external_v. --- mx_util.c | 22 ---------------------- mx_util.h | 1 - 2 files changed, 23 deletions(-) diff --git a/mx_util.c b/mx_util.c index 51f262f8..a718a96e 100644 --- a/mx_util.c +++ b/mx_util.c @@ -1384,25 +1384,3 @@ char *mx_pipe_external_v(char *helper, char **argv) { errno = err; return NULL; } - -char *mx_pipe_external(char *cmd, ...) { - int len = 0; - va_list ap, ap2; - char **argv; - - va_start(ap, cmd); - va_copy(ap2, ap); - do - len++; - while (va_arg(ap, char *) != NULL); - va_end(ap); - len++; - argv=alloca(len*sizeof(*argv)); - int i=0; - argv[i++] = cmd; - do { - argv[i++] = va_arg(ap2, char *); - } while (i Date: Thu, 5 May 2022 18:54:15 +0200 Subject: [PATCH 22/56] mx_util: Rename mx_pipe_external_v to mx_pipe_external --- mx_util.c | 2 +- mx_util.h | 2 +- mxqd.c | 8 ++++---- test_mx_util.c | 14 +++++++------- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/mx_util.c b/mx_util.c index a718a96e..82d8f4d7 100644 --- a/mx_util.c +++ b/mx_util.c @@ -1330,7 +1330,7 @@ static ssize_t readall(int fd, char *buf, size_t buflen) { return len; } -char *mx_pipe_external_v(char *helper, char **argv) { +char *mx_pipe_external(char *helper, char **argv) { int pipefd[2]; int err; char buf[2048]; diff --git a/mx_util.h b/mx_util.h index 2cf78aa9..d5eeb65b 100644 --- a/mx_util.h +++ b/mx_util.h @@ -180,6 +180,6 @@ void _mx_sort_linked_list(void **list, int (*cmp)(void *o1,void *o2), void ** (* unsigned long mx_df(const char *path); time_t mx_clock_boottime(void); -char *mx_pipe_external_v(char *args, char **argv); +char *mx_pipe_external(char *args, char **argv); #endif diff --git a/mxqd.c b/mxqd.c index 8c37c0e8..cec683bd 100644 --- a/mxqd.c +++ b/mxqd.c @@ -324,7 +324,7 @@ static int cpuset_init(struct mxq_server *server) static void read_hostconfig_retry(struct keywordset *kws) { char *argv[] = { "/usr/sbin/hostconfig", NULL }; - char *line = mx_pipe_external_v("/usr/sbin/hostconfig", argv); + char *line = mx_pipe_external("/usr/sbin/hostconfig", argv); if (!line) { mx_log_err("hostconfig: %m"); exit(1); @@ -337,7 +337,7 @@ static char gpu_setup_script[] = LIBEXECDIR "/mxq/gpu-setup"; static int get_gpus(void) { char *argv[] = { gpu_setup_script, "init", NULL }; - char *line = mx_pipe_external_v(gpu_setup_script, argv); + char *line = mx_pipe_external(gpu_setup_script, argv); if (!line) { mx_log_err("gpu-setup init: %m"); exit(1); @@ -910,7 +910,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) mx_asprintf_forever("%u", group->user_uid), NULL }; - char *gpu_uuid = mx_pipe_external_v(gpu_setup_script, argv); + char *gpu_uuid = mx_pipe_external(gpu_setup_script, argv); if (!gpu_uuid) { mx_log_err("gpu-setup job-init: %m"); exit(1); @@ -2053,7 +2053,7 @@ static void release_gpu(struct mxq_server *server, struct mxq_group *group, stru mx_asprintf_forever("%d", job->host_pid), NULL }; - char *gpu_uuid = mx_pipe_external_v(gpu_setup_script, argv); + char *gpu_uuid = mx_pipe_external(gpu_setup_script, argv); free(argv[2]); if (!gpu_uuid) { mx_log_err("gpu-setup job-release: %m"); diff --git a/test_mx_util.c b/test_mx_util.c index 6042cbf1..387db1f6 100644 --- a/test_mx_util.c +++ b/test_mx_util.c @@ -529,28 +529,28 @@ static void test_mx_pipe_external(void) { { char *argv[] = { "/usr/bin/echo", "123", NULL }; - line = mx_pipe_external_v("/usr/bin/echo", argv); + line = mx_pipe_external("/usr/bin/echo", argv); assert(line && strcmp(line, "123") == 0); free(line); } { char *argv[] = { "/usr/bin/echo", "-n", "123", NULL }; - line = mx_pipe_external_v("/usr/bin/echo", argv); + line = mx_pipe_external("/usr/bin/echo", argv); assert(line && strcmp(line, "123") == 0); free(line); } { char *argv[] = { "/usr/bin/echo", "-ne", "123\n456\n\n", NULL }; - line = mx_pipe_external_v("/usr/bin/echo", argv); + line = mx_pipe_external("/usr/bin/echo", argv); assert(line && strcmp(line, "123\n456\n") == 0); free(line); } { char *argv[] = { "/usr/bin/true", NULL }; - line = mx_pipe_external_v("/usr/bin/true", argv); + line = mx_pipe_external("/usr/bin/true", argv); assert(line && strcmp(line, "") == 0); free(line); } @@ -559,19 +559,19 @@ static void test_mx_pipe_external(void) { { char *argv[] = { "/usr/bin/false", NULL }; - line = mx_pipe_external_v("/usr/bin/false", argv); + line = mx_pipe_external("/usr/bin/false", argv); assert(line == NULL && errno==EPROTO); } { char *argv[] = { "/usr/bin/cat", "/usr/bin/bash", NULL }; - line = mx_pipe_external_v("/usr/bin/cat", argv); + line = mx_pipe_external("/usr/bin/cat", argv); assert(line == NULL && errno==EPROTO); } { char *argv[] = { "/usr/bin/yes", NULL }; - line = mx_pipe_external_v("/usr/bin/yes", argv); + line = mx_pipe_external("/usr/bin/yes", argv); assert(line == NULL && errno==EPROTO); } } From 07138665511a4eef185a93c39288b403c2e6802e Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 11:33:27 +0200 Subject: [PATCH 23/56] mx_util: Add mx_call_external --- mx_util.c | 18 ++++++++++++++++++ mx_util.h | 1 + 2 files changed, 19 insertions(+) diff --git a/mx_util.c b/mx_util.c index 82d8f4d7..d04d21dc 100644 --- a/mx_util.c +++ b/mx_util.c @@ -1315,6 +1315,24 @@ time_t mx_clock_boottime(void) { return (ts.tv_sec); } +int mx_call_external(char *helper, char **argv) { + pid_t pid = fork(); + if (pid == 0) { + execv(helper, argv); + mx_log_err("%s: %m", helper); + _exit(1); + } + if (pid == -1) + return -1; + int wstatus; + waitpid(pid, &wstatus, 0); + if (wstatus != 0) { + errno = EPROTO; + return -1; + } + return 0; +} + static ssize_t readall(int fd, char *buf, size_t buflen) { ssize_t len = 0; while (buflen) { diff --git a/mx_util.h b/mx_util.h index d5eeb65b..349e5717 100644 --- a/mx_util.h +++ b/mx_util.h @@ -180,6 +180,7 @@ void _mx_sort_linked_list(void **list, int (*cmp)(void *o1,void *o2), void ** (* unsigned long mx_df(const char *path); time_t mx_clock_boottime(void); +int mx_call_external(char *helper, char **argv); char *mx_pipe_external(char *args, char **argv); #endif From bd856d7f73f583b7ce304272ccf99b18fa0daabb Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 15:10:14 +0200 Subject: [PATCH 24/56] test_mx_util: Add test for mx_call_external --- test_mx_util.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test_mx_util.c b/test_mx_util.c index 387db1f6..9ecf5343 100644 --- a/test_mx_util.c +++ b/test_mx_util.c @@ -576,6 +576,19 @@ static void test_mx_pipe_external(void) { } } +static void test_mx_call_external(void) { + int sts; + errno = 999; + + sts = mx_call_external("/usr/bin/true", NULL); + assert(sts == 0); + assert(errno == 999); + + sts = mx_call_external("/usr/bin/false", NULL); + assert(sts == -1); + assert(errno == EPROTO); +} + int main(void) { test_mx_strskipwhitespaces(); @@ -597,5 +610,6 @@ int main(void) test_listsort(); test_mx_df(); test_mx_pipe_external(); + test_mx_call_external(); return 0; } From 4040371a28f56db18c0a7f9b27fcf2275fc6c9b1 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 19:07:25 +0200 Subject: [PATCH 25/56] helper/create-job-tmpdir: Fix typo in error path --- helper/create_job_tmpdir | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/create_job_tmpdir b/helper/create_job_tmpdir index 3d74e3e1..01207427 100755 --- a/helper/create_job_tmpdir +++ b/helper/create_job_tmpdir @@ -37,6 +37,6 @@ if fallocate -l ${MXQ_SIZE}G $filename; then fi rm $filename else - test -e $fileame && rm $filename + test -e $filename && rm $filename fi exit $status From 06fa4f193919b72b99f78551234c2fefc5130485 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 12:52:51 +0200 Subject: [PATCH 26/56] helper: Rename create_job_tmpdir to tmpdir-setup Rename helper/create_job_tmpdir to helper/tmpdir-setup. Leave the obsolete helper script around to support a rolling upgrade. --- Makefile | 4 ++-- helper/tmpdir-setup | 42 ++++++++++++++++++++++++++++++++++++++++++ mxqd.c | 8 ++++---- 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100755 helper/tmpdir-setup diff --git a/Makefile b/Makefile index ec34bde5..4acc4afc 100644 --- a/Makefile +++ b/Makefile @@ -658,8 +658,8 @@ clean: CLEAN += mxqps ### script helper ----------------------------------------------------- -install:: helper/create_job_tmpdir - $(call quiet-install,0755,$^,${DESTDIR}${LIBEXECDIR}/mxq/create_job_tmpdir) +install:: helper/tmpdir-setup + $(call quiet-install,0755,$^,${DESTDIR}${LIBEXECDIR}/mxq/tmpdir-setup) install:: helper/gpu-setup $(call quiet-install,0755,$^,${DESTDIR}${LIBEXECDIR}/mxq/gpu-setup) diff --git a/helper/tmpdir-setup b/helper/tmpdir-setup new file mode 100755 index 00000000..01207427 --- /dev/null +++ b/helper/tmpdir-setup @@ -0,0 +1,42 @@ +#! /usr/bin/bash + +# Input (environment): +# +# MXQ_JOBID : job ident +# MXQ_SIZE : size in GB +# MXQ_UID : uid + +# Output: +# +# /dev/shm/mxqd/tmp/$JOBID mounted, space from /scratch/local2 + +tmpdir=/scratch/local2/mxqd/tmp +mntdir=/dev/shm/mxqd/mnt/job +filename=$tmpdir/$MXQ_JOBID.tmp +mountpoint=$mntdir/$MXQ_JOBID + +umask 006 +mkdir -p $tmpdir +mkdir -p $mntdir + +status=1; + +if fallocate -l ${MXQ_SIZE}G $filename; then + if loopdevice=$(losetup --find --show $filename); then + if mkfs.ext4 \ + -q \ + -m 0 \ + -E nodiscard,mmp_update_interval=300,lazy_journal_init=1,root_owner=$MXQ_UID:0 \ + -O '64bit,ext_attr,filetype,^has_journal,huge_file,inline_data,^mmp,^quota,sparse_super2' \ + $loopdevice \ + && mkdir -p $mountpoint && mount -Odata=writeback,barrier=0 $loopdevice $mountpoint; then + rmdir $mountpoint/lost+found + status=0 + fi + losetup -d $loopdevice + fi + rm $filename +else + test -e $filename && rm $filename +fi +exit $status diff --git a/mxqd.c b/mxqd.c index cec683bd..098595cf 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1288,7 +1288,7 @@ static unsigned long start_job(struct mxq_group_list *glist) struct mxq_daemon *daemon; - static char create_job_tmpdir_script[] = LIBEXECDIR "/mxq/create_job_tmpdir"; + static char tmpdir_script[] = LIBEXECDIR "/mxq/tmpdir-setup"; pid_t pid; int res; @@ -1314,14 +1314,14 @@ static unsigned long start_job(struct mxq_group_list *glist) if (pid==0) { char *argv[2]; char *envp[4]; - argv[0] = create_job_tmpdir_script, + argv[0] = tmpdir_script, argv[1] = NULL; envp[0] = mx_asprintf_forever("MXQ_JOBID=%lu", job->job_id); envp[1] = mx_asprintf_forever("MXQ_SIZE=%u", group->job_tmpdir_size); envp[2] = mx_asprintf_forever("MXQ_UID=%d", group->user_uid); envp[3] = NULL; - execve(create_job_tmpdir_script,argv,envp); - mx_log_fatal("exec %s : %m",create_job_tmpdir_script); + execve(tmpdir_script, argv,envp); + mx_log_fatal("exec %s : %m", tmpdir_script); exit(1); } mx_mysql_connect_forever(&(server->mysql)); From 40de658a3677e47b24d14725fee377883d2e6f86 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 13:29:20 +0200 Subject: [PATCH 27/56] tmpdir-setup: Use arguments instead of environment Change the tmdir-setup script helper to get ist arguments from its argument list not from the environment. Change mxqd to use the mx_call_external helper to call it. --- helper/tmpdir-setup | 88 +++++++++++++++++++++++++++------------------ mxqd.c | 36 ++++++++----------- 2 files changed, 67 insertions(+), 57 deletions(-) diff --git a/helper/tmpdir-setup b/helper/tmpdir-setup index 01207427..aab193f4 100755 --- a/helper/tmpdir-setup +++ b/helper/tmpdir-setup @@ -1,42 +1,60 @@ #! /usr/bin/bash -# Input (environment): -# -# MXQ_JOBID : job ident -# MXQ_SIZE : size in GB -# MXQ_UID : uid - -# Output: -# -# /dev/shm/mxqd/tmp/$JOBID mounted, space from /scratch/local2 +usage() { + cat <&2 +usage: + $0 create JOBID SIZE UID # create SIZE GiB /dev/shm/mxqd/mnt/job/$JOBID +EOF + exit 1 +} tmpdir=/scratch/local2/mxqd/tmp mntdir=/dev/shm/mxqd/mnt/job -filename=$tmpdir/$MXQ_JOBID.tmp -mountpoint=$mntdir/$MXQ_JOBID - -umask 006 -mkdir -p $tmpdir -mkdir -p $mntdir - -status=1; - -if fallocate -l ${MXQ_SIZE}G $filename; then - if loopdevice=$(losetup --find --show $filename); then - if mkfs.ext4 \ - -q \ - -m 0 \ - -E nodiscard,mmp_update_interval=300,lazy_journal_init=1,root_owner=$MXQ_UID:0 \ - -O '64bit,ext_attr,filetype,^has_journal,huge_file,inline_data,^mmp,^quota,sparse_super2' \ - $loopdevice \ - && mkdir -p $mountpoint && mount -Odata=writeback,barrier=0 $loopdevice $mountpoint; then - rmdir $mountpoint/lost+found - status=0 + +cmd_create() { + (( $# == 3 )) || usage + MXQ_JOBID=$1 + MXQ_SIZE=$2 + MXQ_UID=$3 + + filename=$tmpdir/$MXQ_JOBID.tmp + mountpoint=$mntdir/$MXQ_JOBID + + umask 006 + mkdir -p $tmpdir + mkdir -p $mntdir + + status=1; + + if fallocate -l ${MXQ_SIZE}G $filename; then + if loopdevice=$(losetup --find --show $filename); then + if mkfs.ext4 \ + -q \ + -m 0 \ + -E nodiscard,mmp_update_interval=300,lazy_journal_init=1,root_owner=$MXQ_UID:0 \ + -O '64bit,ext_attr,filetype,^has_journal,huge_file,inline_data,^mmp,^quota,sparse_super2' \ + $loopdevice \ + && mkdir -p $mountpoint && mount -Odata=writeback,barrier=0 $loopdevice $mountpoint; then + rmdir $mountpoint/lost+found + status=0 + fi + losetup -d $loopdevice fi - losetup -d $loopdevice + rm $filename + else + test -e $filename && rm $filename fi - rm $filename -else - test -e $filename && rm $filename -fi -exit $status + exit $status +} + +(( $# > 0 )) || usage +cmd="$1" +shift; +case "$cmd" in + create) + cmd_create "$@" + ;; + *) + usage + ;; +esac diff --git a/mxqd.c b/mxqd.c index 098595cf..6bb06af0 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1292,7 +1292,6 @@ static unsigned long start_job(struct mxq_group_list *glist) pid_t pid; int res; - int status; assert(glist); assert(glist->user); @@ -1310,28 +1309,21 @@ static unsigned long start_job(struct mxq_group_list *glist) if (group->job_tmpdir_size > 0) { mx_mysql_disconnect(server->mysql); - pid = fork(); - if (pid==0) { - char *argv[2]; - char *envp[4]; - argv[0] = tmpdir_script, - argv[1] = NULL; - envp[0] = mx_asprintf_forever("MXQ_JOBID=%lu", job->job_id); - envp[1] = mx_asprintf_forever("MXQ_SIZE=%u", group->job_tmpdir_size); - envp[2] = mx_asprintf_forever("MXQ_UID=%d", group->user_uid); - envp[3] = NULL; - execve(tmpdir_script, argv,envp); - mx_log_fatal("exec %s : %m", tmpdir_script); - exit(1); - } + char *argv[] = { + tmpdir_script, + "create", + mx_asprintf_forever("%lu", job->job_id), + mx_asprintf_forever("%u", group->job_tmpdir_size), + mx_asprintf_forever("%d", group->user_uid), + NULL + }; + int status = mx_call_external(tmpdir_script, argv); + free(argv[2]); + free(argv[3]); + free(argv[4]); mx_mysql_connect_forever(&(server->mysql)); - if (pid < 0) { - mx_log_err("fork: %m"); - mxq_unload_job_from_server(server->mysql, job->job_id); - return(0); - } - waitpid(pid, &status, 0); - if (status) { + if (status == -1) { + mx_log_err("create job tmpdir: %m"); mxq_unload_job_from_server(server->mysql, job->job_id); return 0; } From 84a66e45ec70a30eb73194c8bd6fde48403d5d79 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 18:35:58 +0200 Subject: [PATCH 28/56] mxqd: Do not disconnect from mysql before helper call The helpers are trusted code, they won't do something nasty with the connected mysql socket. So don't disconnect from and reconnect to the mysql server when we call the tmpdir-setup helper. --- mxqd.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/mxqd.c b/mxqd.c index 6bb06af0..01d5d161 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1308,7 +1308,6 @@ static unsigned long start_job(struct mxq_group_list *glist) } if (group->job_tmpdir_size > 0) { - mx_mysql_disconnect(server->mysql); char *argv[] = { tmpdir_script, "create", @@ -1321,7 +1320,6 @@ static unsigned long start_job(struct mxq_group_list *glist) free(argv[2]); free(argv[3]); free(argv[4]); - mx_mysql_connect_forever(&(server->mysql)); if (status == -1) { mx_log_err("create job tmpdir: %m"); mxq_unload_job_from_server(server->mysql, job->job_id); From f12d171141b87faaaafc4d505b900d2a4ad8bbb2 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Fri, 15 Apr 2022 20:20:29 +0200 Subject: [PATCH 29/56] mxqd: Sleep, when external helper failed to set up tmpdir Throttle main loop in case this is a systematic error. --- mxqd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mxqd.c b/mxqd.c index 01d5d161..0fd06c58 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1323,6 +1323,7 @@ static unsigned long start_job(struct mxq_group_list *glist) if (status == -1) { mx_log_err("create job tmpdir: %m"); mxq_unload_job_from_server(server->mysql, job->job_id); + sleep(30); return 0; } } From f752a601bf2a2afbb4a5f9be39a183cd2ae6191b Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 19:04:33 +0200 Subject: [PATCH 30/56] Add mxq_reaper Add new external helper which is to be used as the process image of the reaper process. By replacing the memory image of mxqd in the long-running reaper, we decrease our memory footprint and avoid problems with undefined behaviour of the libraries (mysql, openssl) we use. Only put functions from mxqd into this program, which need to go here: - Set our thread name (again, because execl() will rest it) so that mxqd can identify us as a reaper process - Fork a user process - Change uid of the user process. mxqd needs to start us with a privileged uid, so that we can open the spool file. - Let the child process execute the user command and arguments - Wait for the child process - get its runtime, resource usage and exit status - Delay, if child finished to fast - Write spool file for mxqd to pick up Everything else (groupid, subreaper, environment, stdout, ...) can be done by mxqd before exec()ing this helper. --- .gitignore | 2 + Makefile | 7 ++++ mxq_reaper.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+) create mode 100644 mxq_reaper.c diff --git a/.gitignore b/.gitignore index 61b51d6d..7a5de194 100644 --- a/.gitignore +++ b/.gitignore @@ -30,6 +30,7 @@ parser.tab.o test_parser.o test_parser ppidcache.o +mxq_reaper.o mxqsub /mxqsub.1 @@ -44,6 +45,7 @@ test_mx_log test_mx_mysq test_mxqd_control test_keywordset +mxq_reaper /web/pages/mxq/mxq web/lighttpd.conf diff --git a/Makefile b/Makefile index 4acc4afc..2f7a6c0f 100644 --- a/Makefile +++ b/Makefile @@ -656,6 +656,13 @@ build: mxqps clean: CLEAN += mxqps +### mxqps ------------------------------------------------------------- + +clean: CLEAN += mxq_reaper.o mxq_reaper +build: mxq_reaper +install:: mxq_reaper + $(call quiet-install,0755,$^,${DESTDIR}${LIBEXECDIR}/mxq/mxq_reaper) + ### script helper ----------------------------------------------------- install:: helper/tmpdir-setup diff --git a/mxq_reaper.c b/mxq_reaper.c new file mode 100644 index 00000000..f843980a --- /dev/null +++ b/mxq_reaper.c @@ -0,0 +1,104 @@ +#define _GNU_SOURCE +#include +#include +#include +#include +#include +#include +#include +#include +#include + +static const char REAPER_PNAME[] = "mxqd reaper"; + +__attribute__((noreturn)) static void die(char *msg) { + perror(msg); + _exit(1); +} + +int main(int argc, char **argv) { + + if (argc < 5 || strcmp(argv[3], "--") != 0) { + fprintf(stderr, "usage: %s UID SPOOLFILENAME -- CMD [ARGS...]\n", argv[0]); + exit(1); + } + + uid_t uid = atoi(argv[1]); + char *spoolfilename = argv[2]; + char **user_argv = &argv[4]; + + pid_t user_pid; + int user_status = -1; + struct rusage user_rusage; + struct timeval user_time; + + struct timeval starttime; + struct timeval endtime; + + if (prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL) == -1) + die("PR_SET_NAME"); + user_pid = fork(); + if (user_pid == 0) { + if (setreuid(uid, uid) == -1) + die("setreuid"); + execvp(user_argv[0], user_argv); + die(user_argv[0]); + } + if (user_pid == -1) + die("fork"); + if (gettimeofday(&starttime, NULL) == -1) + die("gettimeofday"); + while (1) { + int status; + pid_t pid = wait(&status); + if (pid < 0 && errno == ECHILD) + break; + if (pid == user_pid) + user_status = status; + } + if (gettimeofday(&endtime, NULL) == -1) + die("gettimeofday"); + timersub(&endtime, &starttime, &user_time); + if (getrusage(RUSAGE_CHILDREN, &user_rusage) == -1) + die("getrusage"); + + if (user_time.tv_sec<30) { + int wait=30-user_time.tv_sec; + sleep(wait); + } + + char *tmpfilename; + if (asprintf(&tmpfilename, "%s.tmp", spoolfilename) == -1) + die(""); + + FILE *out = fopen(tmpfilename,"w"); + if (out == NULL) + die(tmpfilename); + fprintf(out,"1 %d %d %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld\n", + getpid(), + user_status, + user_time.tv_sec, user_time.tv_usec, + user_rusage.ru_utime.tv_sec, user_rusage.ru_utime.tv_usec, + user_rusage.ru_stime.tv_sec, user_rusage.ru_stime.tv_usec, + user_rusage.ru_maxrss, + user_rusage.ru_ixrss, + user_rusage.ru_idrss, + user_rusage.ru_isrss, + user_rusage.ru_minflt, + user_rusage.ru_majflt, + user_rusage.ru_nswap, + user_rusage.ru_inblock, + user_rusage.ru_oublock, + user_rusage.ru_msgsnd, + user_rusage.ru_msgrcv, + user_rusage.ru_nsignals, + user_rusage.ru_nvcsw, + user_rusage.ru_nivcsw + ); + fflush(out); + fsync(fileno(out)); + fclose(out); + if (rename(tmpfilename, spoolfilename) == -1) + die(spoolfilename); + return 0; +} From cba234a0683a05363a093ba30986e5676a2c0097 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 19:05:00 +0200 Subject: [PATCH 31/56] mxqd: Use external reaper Call the external helper instead of running the reaper process in the memory cloned from the mxq main process. - init_child_process() was used to initialize the user process forked from the reaper. We now do the same things in the reaper process itself because we no longer fork the user process in this program, the external reaper image will do that. The settings we do now to the reaper process will be inherited by the user process. - As before we need to change our effective user ident before we chdir into the cwd of the job and open the output files for the user. But we keep the real user ident, so that we can change back to root later. - user_process() now exec()s the external helper with the required arguments instead of the user image directly. Before we do so, we need to change our UIDs back to root. The external helper needs privileges to write the spool file. - The functionaltiy to wait for the user process and write the spool file is removed, this is now done by the external reaper. - In the absense of errors, the function reaper_process() will no longer return. Note: We don't free new_argv in user_process, because we will exec() or _exit() anyway. --- mxqd.c | 121 ++++++++++++++------------------------------------------- 1 file changed, 29 insertions(+), 92 deletions(-) diff --git a/mxqd.c b/mxqd.c index 0fd06c58..54e327a1 100644 --- a/mxqd.c +++ b/mxqd.c @@ -975,7 +975,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) return 0; } - res = setreuid(group->user_uid, group->user_uid); + res = setreuid(-1, group->user_uid); if (res == -1) { mx_log_err("job=%s(%d):%lu:%lu setreuid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, @@ -1090,7 +1090,7 @@ static int mxq_redirect_input(char *stdin_fname) return 1; } -static int user_process(struct mxq_group_list *glist, struct mxq_job *job) +static int user_process(struct mxq_server *server, struct mxq_group_list *glist, struct mxq_job *job) { int res; char **argv; @@ -1138,7 +1138,30 @@ static int user_process(struct mxq_group_list *glist, struct mxq_job *job) return -errno; } - res = execvp(argv[0], argv); + int argc = 0; + while (argv[argc] != NULL) + argc++; + + char **new_argv = mx_calloc_forever(argc+4+1, sizeof(char *)); + new_argv[0] = LIBEXECDIR "/mxq/mxq_reaper"; + new_argv[1] = mx_asprintf_forever("%d", group->user_uid); + new_argv[2] = mx_asprintf_forever("%s/%lu.stat", server->finished_jobsdir, job->job_id); + new_argv[3] = "--"; + for (int i = 0; i < argc ; i++) + new_argv[i+4] = argv[i]; + new_argv[argc+4] = NULL; + + res = setuid(0); + if (res == -1) { + mx_log_err("job=%s(%d):%lu:%lu setuid(0) failed: %m", + group->user_name, + group->user_uid, + group->group_id, + job->job_id); + return -errno; + } + + res = execvp(new_argv[0], new_argv); mx_log_err("job=%s(%d):%lu:%lu execvp(\"%s\", ...): %m", group->user_name, group->user_uid, @@ -1161,16 +1184,6 @@ static int is_reaper(pid_t pid) { } static int reaper_process(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { - pid_t pid; - struct rusage rusage; - int status = 0; - pid_t waited_pid; - int waited_status; - struct timeval now; - struct timeval realtime; - _mx_cleanup_free_ char *finished_job_filename=NULL; - _mx_cleanup_free_ char *finished_job_tmpfilename=NULL; - FILE *out; int res; struct mxq_group *group; @@ -1195,85 +1208,9 @@ static int reaper_process(struct mxq_server *server,struct mxq_group_list *glist return res; } - pid = fork(); - if (pid < 0) { - mx_log_err("fork: %m"); - return pid; - } else if (pid == 0) { - mx_log_debug("starting user process."); - res = user_process(glist, job); - _exit(EX__MAX+1); - } - gettimeofday(&job->stats_starttime, NULL); - - while (1) { - waited_pid = wait(&waited_status); - if (waited_pid < 0) { - if (errno==ECHILD) { - break; - } else { - mx_log_warning("reaper: wait: %m"); - sleep(1); - } - } - if (waited_pid == pid) { - status = waited_status; - } - } - gettimeofday(&now, NULL); - timersub(&now, &job->stats_starttime, &realtime); - - if (realtime.tv_sec<30) { - int wait=30-realtime.tv_sec; - mx_log_warning("user process finished to fast (%ld seconds) : delaying termination for %d seconds",realtime.tv_sec,wait); - sleep(wait); - } - - res = getrusage(RUSAGE_CHILDREN, &rusage); - if (res < 0) { - mx_log_err("reaper: getrusage: %m"); - return(res); - } - - finished_job_filename = mx_asprintf_forever("%s/%lu.stat", server->finished_jobsdir, job->job_id); - finished_job_tmpfilename = mx_asprintf_forever("%s.tmp", finished_job_filename); - - out=fopen(finished_job_tmpfilename,"w"); - if (!out) { - mx_log_fatal("%s: %m",finished_job_tmpfilename); - return (-errno); - } - - fprintf(out,"1 %d %d %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld %ld\n", - getpid(), - status, - realtime.tv_sec,realtime.tv_usec, - rusage.ru_utime.tv_sec,rusage.ru_utime.tv_usec, - rusage.ru_stime.tv_sec,rusage.ru_stime.tv_usec, - rusage.ru_maxrss, - rusage.ru_ixrss, - rusage.ru_idrss, - rusage.ru_isrss, - rusage.ru_minflt, - rusage.ru_majflt, - rusage.ru_nswap, - rusage.ru_inblock, - rusage.ru_oublock, - rusage.ru_msgsnd, - rusage.ru_msgrcv, - rusage.ru_nsignals, - rusage.ru_nvcsw, - rusage.ru_nivcsw - ); - fflush(out); - fsync(fileno(out)); - fclose(out); - res=rename(finished_job_tmpfilename,finished_job_filename); - if (res<0) { - mx_log_fatal("rename %s: %m",finished_job_tmpfilename); - return(res); - } - return(0); + res = user_process(server, glist, job); + mx_log_err("user process:: %m"); + return res; } static unsigned long start_job(struct mxq_group_list *glist) From aaa6c77514453d095f63ca8fc4fb511242064326 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 21:07:30 +0200 Subject: [PATCH 32/56] mxqd: Rename reaper_process to exec_reaper Rename the function to highlight the fact that it doesn't return in the absence of errors. --- mxqd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mxqd.c b/mxqd.c index 54e327a1..783b2e0f 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1183,7 +1183,7 @@ static int is_reaper(pid_t pid) { return 0; } -static int reaper_process(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { +static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { int res; struct mxq_group *group; @@ -1285,7 +1285,7 @@ static unsigned long start_job(struct mxq_group_list *glist) server->flock = NULL; mx_mysql_finish(&server->mysql); - res = reaper_process(server, glist, job); + res = exec_reaper(server, glist, job); mxq_job_free_content(job); From 30cdd486af6823eb950e03ff5df40e023fa2dd02 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 21:21:34 +0200 Subject: [PATCH 33/56] mxqd: Inline user_process into exec_reaper --- mxqd.c | 78 ++++++++++++++++++++++++---------------------------------- 1 file changed, 32 insertions(+), 46 deletions(-) diff --git a/mxqd.c b/mxqd.c index 783b2e0f..4e4511bd 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1090,15 +1090,43 @@ static int mxq_redirect_input(char *stdin_fname) return 1; } -static int user_process(struct mxq_server *server, struct mxq_group_list *glist, struct mxq_job *job) -{ +static const char REAPER_PNAME[] = "mxqd reaper"; + +static int is_reaper(pid_t pid) { + char comm[16]; + if (mx_proc_get_comm(pid, comm) == NULL) + return 0; + if (strcmp(comm, REAPER_PNAME) == 0) + return 1; + else + return 0; +} + +static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { int res; - char **argv; struct mxq_group *group; group = &glist->group; + res = prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL); + if (res < 0) { + mx_log_err("reaper_process set name: %m"); + return res; + } + + res = setsid(); + if (res < 0) { + mx_log_err("reaper_process setsid: %m"); + return res; + } + + res = prctl(PR_SET_CHILD_SUBREAPER, 1); + if (res < 0) { + mx_log_err("set subreaper: %m"); + return res; + } + res = init_child_process(glist, job); if (!res) return(-1); @@ -1127,7 +1155,7 @@ static int user_process(struct mxq_server *server, struct mxq_group_list *glist, return(res); } - argv = mx_strvec_from_str(job->job_argv_str); + char **argv = mx_strvec_from_str(job->job_argv_str); if (!argv) { mx_log_err("job=%s(%d):%lu:%lu Can't recaculate commandline. str_to_strvev(%s) failed: %m", group->user_name, @@ -1171,48 +1199,6 @@ static int user_process(struct mxq_server *server, struct mxq_group_list *glist, return res; } -static const char REAPER_PNAME[] = "mxqd reaper"; - -static int is_reaper(pid_t pid) { - char comm[16]; - if (mx_proc_get_comm(pid, comm) == NULL) - return 0; - if (strcmp(comm, REAPER_PNAME) == 0) - return 1; - else - return 0; -} - -static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { - int res; - - struct mxq_group *group; - - group = &glist->group; - - res = prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL); - if (res < 0) { - mx_log_err("reaper_process set name: %m"); - return res; - } - - res = setsid(); - if (res < 0) { - mx_log_err("reaper_process setsid: %m"); - return res; - } - - res = prctl(PR_SET_CHILD_SUBREAPER, 1); - if (res < 0) { - mx_log_err("set subreaper: %m"); - return res; - } - - res = user_process(server, glist, job); - mx_log_err("user process:: %m"); - return res; -} - static unsigned long start_job(struct mxq_group_list *glist) { struct mxq_server *server; From eee337d528811eb1edcaa528a94fec543b074d5f Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 21:27:52 +0200 Subject: [PATCH 34/56] mxqd: Invert return status of init_child_process Return 0 on success, -1 on failure. This change will make the following commit easier to follow. --- mxqd.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/mxqd.c b/mxqd.c index 4e4511bd..405f50d6 100644 --- a/mxqd.c +++ b/mxqd.c @@ -835,7 +835,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) if (!passwd) { mx_log_err("job=%s(%d):%lu:%lu getpwuid(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return 0; + return -1; } if (!mx_streq(passwd->pw_name, group->user_name)) { @@ -852,7 +852,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) if (!passwd) { mx_log_err("job=%s(%d):%lu:%lu getpwnam(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return 0; + return -1; } if (passwd->pw_uid != group->user_uid) { mx_log_fatal("job=%s(%d):%lu:%lu user_name=%s does not map to uid=%d but to pw_uid=%d. Aborting Child execution.", @@ -864,7 +864,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) group->user_uid, passwd->pw_uid); - return 0; + return -1; } } @@ -874,7 +874,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) if (res != 0) { mx_log_err("job=%s(%d):%lu:%lu clearenv(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return 0; + return -1; } mx_setenv_forever("USER", group->user_name); @@ -925,7 +925,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) if (fh == -1) { mx_log_err("job=%s(%d):%lu:%lu open(%s) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, "/proc/self/loginuid"); - return 0; + return -1; } dprintf(fh, "%d", group->user_uid); close(fh); @@ -964,7 +964,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) if (res == -1) { mx_log_err("job=%s(%d):%lu:%lu initgroups() failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return 0; + return -1; } res = setregid(group->user_gid, group->user_gid); @@ -972,7 +972,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) mx_log_err("job=%s(%d):%lu:%lu setregid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, group->user_gid, group->user_gid); - return 0; + return -1; } res = setreuid(-1, group->user_uid); @@ -980,7 +980,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) mx_log_err("job=%s(%d):%lu:%lu setreuid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, group->user_uid, group->user_uid); - return 0; + return -1; } res = chdir(job->job_workdir); @@ -988,7 +988,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) mx_log_err("job=%s(%d):%lu:%lu chdir(%s) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, job->job_workdir); - return 0; + return -1; } umask(job->job_umask); @@ -996,7 +996,7 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) res=sched_setaffinity(0,sizeof(job->host_cpu_set),&job->host_cpu_set); if (res<0) mx_log_warning("sched_setaffinity: $m"); - return 1; + return 0; } /**********************************************************************/ @@ -1128,7 +1128,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s } res = init_child_process(glist, job); - if (!res) + if (res == -1) return(-1); mxq_job_set_tmpfilenames(group, job); From c2f254e29189d0b7f98479a84a3dc0fa733f8ecf Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 21:32:28 +0200 Subject: [PATCH 35/56] mxqd: Inline init_child_process into exec_reaper --- mxqd.c | 270 ++++++++++++++++++++++++++------------------------------- 1 file changed, 124 insertions(+), 146 deletions(-) diff --git a/mxqd.c b/mxqd.c index 405f50d6..c1ba5dc2 100644 --- a/mxqd.c +++ b/mxqd.c @@ -811,22 +811,135 @@ static int server_init(struct mxq_server *server, int argc, char *argv[]) return 0; } -static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) +static int mxq_redirect_open(char *fname) { - struct mxq_server *server; - struct mxq_group *group; - struct passwd *passwd; + int fh; int res; + + int flags = O_WRONLY|O_CREAT|O_NOFOLLOW|O_TRUNC; + mode_t mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH; + + + if (!fname) { + fname = "/dev/null"; + } else if (!mx_streq(fname, "/dev/null")) { + res = unlink(fname); + if (res == -1 && errno != ENOENT) { + mx_log_err("%s: unlink() failed: %m", fname); + return -2; + } + flags |= O_EXCL; + } + + fh = open(fname, flags, mode); + if (fh == -1) { + mx_log_err("open() failed: %m"); + } + + return fh; + +} + +static int mxq_redirect(char *fname, int fd) +{ int fh; - struct rlimit rlim; + int res; - assert(job); - assert(glist); - assert(glist->user); - assert(glist->user->server); + fh = mxq_redirect_open(fname); + if (fh < 0) + return -1; - server = glist->user->server; - group = &glist->group; + res = mx_dup2_close_both(fh, fd); + if (res < 0) + return -2; + + return 0; +} + +static int mxq_redirect_output(char *stdout_fname, char *stderr_fname) +{ + int res; + + res = mxq_redirect(stderr_fname, STDERR_FILENO); + if (res < 0) { + return -1; + } + + if (stdout_fname == stderr_fname) { + res = mx_dup2_close_new(STDERR_FILENO, STDOUT_FILENO); + if( res < 0) { + return -2; + } + return 0; + } + + res = mxq_redirect(stdout_fname, STDOUT_FILENO); + if (res < 0) { + return -3; + } + + return 0; +} + +static int mxq_redirect_input(char *stdin_fname) +{ + int fh; + int res; + + fh = open(stdin_fname, O_RDONLY|O_NOFOLLOW); + if (fh == -1) { + mx_log_err("open() failed: %m"); + return -1; + } + + res = mx_dup2_close_both(fh, STDIN_FILENO); + if (res < 0) { + return -2; + } + + return 1; +} + +static const char REAPER_PNAME[] = "mxqd reaper"; + +static int is_reaper(pid_t pid) { + char comm[16]; + if (mx_proc_get_comm(pid, comm) == NULL) + return 0; + if (strcmp(comm, REAPER_PNAME) == 0) + return 1; + else + return 0; +} + +static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { + int res; + + struct mxq_group *group; + + group = &glist->group; + + res = prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL); + if (res < 0) { + mx_log_err("reaper_process set name: %m"); + return res; + } + + res = setsid(); + if (res < 0) { + mx_log_err("reaper_process setsid: %m"); + return res; + } + + res = prctl(PR_SET_CHILD_SUBREAPER, 1); + if (res < 0) { + mx_log_err("set subreaper: %m"); + return res; + } + + struct passwd *passwd; + int fh; + struct rlimit rlim; sigprocmask(SIG_UNBLOCK,&all_signals,NULL); signal(SIGPIPE,SIG_DFL); @@ -996,141 +1109,6 @@ static int init_child_process(struct mxq_group_list *glist, struct mxq_job *job) res=sched_setaffinity(0,sizeof(job->host_cpu_set),&job->host_cpu_set); if (res<0) mx_log_warning("sched_setaffinity: $m"); - return 0; -} - -/**********************************************************************/ - -static int mxq_redirect_open(char *fname) -{ - int fh; - int res; - - int flags = O_WRONLY|O_CREAT|O_NOFOLLOW|O_TRUNC; - mode_t mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH; - - - if (!fname) { - fname = "/dev/null"; - } else if (!mx_streq(fname, "/dev/null")) { - res = unlink(fname); - if (res == -1 && errno != ENOENT) { - mx_log_err("%s: unlink() failed: %m", fname); - return -2; - } - flags |= O_EXCL; - } - - fh = open(fname, flags, mode); - if (fh == -1) { - mx_log_err("open() failed: %m"); - } - - return fh; - -} - -static int mxq_redirect(char *fname, int fd) -{ - int fh; - int res; - - fh = mxq_redirect_open(fname); - if (fh < 0) - return -1; - - res = mx_dup2_close_both(fh, fd); - if (res < 0) - return -2; - - return 0; -} - -static int mxq_redirect_output(char *stdout_fname, char *stderr_fname) -{ - int res; - - res = mxq_redirect(stderr_fname, STDERR_FILENO); - if (res < 0) { - return -1; - } - - if (stdout_fname == stderr_fname) { - res = mx_dup2_close_new(STDERR_FILENO, STDOUT_FILENO); - if( res < 0) { - return -2; - } - return 0; - } - - res = mxq_redirect(stdout_fname, STDOUT_FILENO); - if (res < 0) { - return -3; - } - - return 0; -} - -static int mxq_redirect_input(char *stdin_fname) -{ - int fh; - int res; - - fh = open(stdin_fname, O_RDONLY|O_NOFOLLOW); - if (fh == -1) { - mx_log_err("open() failed: %m"); - return -1; - } - - res = mx_dup2_close_both(fh, STDIN_FILENO); - if (res < 0) { - return -2; - } - - return 1; -} - -static const char REAPER_PNAME[] = "mxqd reaper"; - -static int is_reaper(pid_t pid) { - char comm[16]; - if (mx_proc_get_comm(pid, comm) == NULL) - return 0; - if (strcmp(comm, REAPER_PNAME) == 0) - return 1; - else - return 0; -} - -static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { - int res; - - struct mxq_group *group; - - group = &glist->group; - - res = prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL); - if (res < 0) { - mx_log_err("reaper_process set name: %m"); - return res; - } - - res = setsid(); - if (res < 0) { - mx_log_err("reaper_process setsid: %m"); - return res; - } - - res = prctl(PR_SET_CHILD_SUBREAPER, 1); - if (res < 0) { - mx_log_err("set subreaper: %m"); - return res; - } - - res = init_child_process(glist, job); - if (res == -1) - return(-1); - mxq_job_set_tmpfilenames(group, job); res = mxq_redirect_input("/dev/null"); From f311ae217e205d7bce19e193d303713d6b230cf3 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 21:43:51 +0200 Subject: [PATCH 36/56] mxqd: Do not return status from exec_reaper exec_reaper only returns on failure, so don't return a status value. --- mxqd.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/mxqd.c b/mxqd.c index c1ba5dc2..14319956 100644 --- a/mxqd.c +++ b/mxqd.c @@ -912,7 +912,7 @@ static int is_reaper(pid_t pid) { return 0; } -static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { +static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { int res; struct mxq_group *group; @@ -922,19 +922,19 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s res = prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL); if (res < 0) { mx_log_err("reaper_process set name: %m"); - return res; + return; } res = setsid(); if (res < 0) { mx_log_err("reaper_process setsid: %m"); - return res; + return; } res = prctl(PR_SET_CHILD_SUBREAPER, 1); if (res < 0) { mx_log_err("set subreaper: %m"); - return res; + return; } struct passwd *passwd; @@ -948,7 +948,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s if (!passwd) { mx_log_err("job=%s(%d):%lu:%lu getpwuid(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return -1; + return; } if (!mx_streq(passwd->pw_name, group->user_name)) { @@ -965,7 +965,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s if (!passwd) { mx_log_err("job=%s(%d):%lu:%lu getpwnam(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return -1; + return; } if (passwd->pw_uid != group->user_uid) { mx_log_fatal("job=%s(%d):%lu:%lu user_name=%s does not map to uid=%d but to pw_uid=%d. Aborting Child execution.", @@ -977,7 +977,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s group->user_uid, passwd->pw_uid); - return -1; + return; } } @@ -987,7 +987,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s if (res != 0) { mx_log_err("job=%s(%d):%lu:%lu clearenv(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return -1; + return; } mx_setenv_forever("USER", group->user_name); @@ -1038,7 +1038,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s if (fh == -1) { mx_log_err("job=%s(%d):%lu:%lu open(%s) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, "/proc/self/loginuid"); - return -1; + return; } dprintf(fh, "%d", group->user_uid); close(fh); @@ -1077,7 +1077,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s if (res == -1) { mx_log_err("job=%s(%d):%lu:%lu initgroups() failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id); - return -1; + return; } res = setregid(group->user_gid, group->user_gid); @@ -1085,7 +1085,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s mx_log_err("job=%s(%d):%lu:%lu setregid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, group->user_gid, group->user_gid); - return -1; + return; } res = setreuid(-1, group->user_uid); @@ -1093,7 +1093,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s mx_log_err("job=%s(%d):%lu:%lu setreuid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, group->user_uid, group->user_uid); - return -1; + return; } res = chdir(job->job_workdir); @@ -1101,7 +1101,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s mx_log_err("job=%s(%d):%lu:%lu chdir(%s) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, job->job_workdir); - return -1; + return; } umask(job->job_umask); @@ -1119,7 +1119,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s group->group_id, job->job_id, res); - return(res); + return; } res = mxq_redirect_output(job->tmp_stdout, job->tmp_stderr); @@ -1130,7 +1130,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s group->group_id, job->job_id, res); - return(res); + return; } char **argv = mx_strvec_from_str(job->job_argv_str); @@ -1141,7 +1141,7 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s group->group_id, job->job_id, job->job_argv_str); - return -errno; + return; } int argc = 0; @@ -1164,17 +1164,16 @@ static int exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, s group->user_uid, group->group_id, job->job_id); - return -errno; + return; } - res = execvp(new_argv[0], new_argv); + execvp(new_argv[0], new_argv); mx_log_err("job=%s(%d):%lu:%lu execvp(\"%s\", ...): %m", group->user_name, group->user_uid, group->group_id, job->job_id, argv[0]); - return res; } static unsigned long start_job(struct mxq_group_list *glist) @@ -1249,14 +1248,14 @@ static unsigned long start_job(struct mxq_group_list *glist) server->flock = NULL; mx_mysql_finish(&server->mysql); - res = exec_reaper(server, glist, job); + exec_reaper(server, glist, job); mxq_job_free_content(job); mx_log_debug("shutting down reaper, bye bye."); mx_log_finish(); server_free(server); - _exit(res<0 ? EX__MAX+1 : 0); + _exit(EX__MAX+1); } gettimeofday(&job->stats_starttime, NULL); From 88d9aa4486863e888d489eb72cd4c21154abd588 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 21:59:37 +0200 Subject: [PATCH 37/56] mxqd: Simplify exec_reaper() --- mxqd.c | 81 +++++++++++++++++----------------------------------------- 1 file changed, 24 insertions(+), 57 deletions(-) diff --git a/mxqd.c b/mxqd.c index 14319956..1519a92e 100644 --- a/mxqd.c +++ b/mxqd.c @@ -913,38 +913,25 @@ static int is_reaper(pid_t pid) { } static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, struct mxq_job *job) { - int res; - - struct mxq_group *group; + struct mxq_group *group = &glist->group; - group = &glist->group; - - res = prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL); - if (res < 0) { + if (prctl(PR_SET_NAME, REAPER_PNAME, NULL, NULL, NULL) ==-1) { mx_log_err("reaper_process set name: %m"); return; } - - res = setsid(); - if (res < 0) { + if (setsid() == -1) { mx_log_err("reaper_process setsid: %m"); return; } - - res = prctl(PR_SET_CHILD_SUBREAPER, 1); - if (res < 0) { + if (prctl(PR_SET_CHILD_SUBREAPER, 1) == -1) { mx_log_err("set subreaper: %m"); return; } - struct passwd *passwd; - int fh; - struct rlimit rlim; - sigprocmask(SIG_UNBLOCK,&all_signals,NULL); signal(SIGPIPE,SIG_DFL); - passwd = getpwuid(group->user_uid); + struct passwd *passwd = getpwuid(group->user_uid); if (!passwd) { mx_log_err("job=%s(%d):%lu:%lu getpwuid(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); @@ -981,10 +968,7 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, } } - /* prepare environment */ - - res = clearenv(); - if (res != 0) { + if (clearenv() != 0) { mx_log_err("job=%s(%d):%lu:%lu clearenv(): %m", group->user_name, group->user_uid, group->group_id, job->job_id); return; @@ -1034,7 +1018,7 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, free(argv[3]); } - fh = open("/proc/self/loginuid", O_WRONLY|O_TRUNC); + int fh = open("/proc/self/loginuid", O_WRONLY|O_TRUNC); if (fh == -1) { mx_log_err("job=%s(%d):%lu:%lu open(%s) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, "/proc/self/loginuid"); @@ -1043,21 +1027,16 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, dprintf(fh, "%d", group->user_uid); close(fh); - /* set memory limits */ + struct rlimit rlim; rlim.rlim_cur = group->job_memory*1024*1024; rlim.rlim_max = group->job_memory*1024*1024; - - res = setrlimit(RLIMIT_DATA, &rlim); - if (res == -1) + if (setrlimit(RLIMIT_DATA, &rlim) == -1) mx_log_err("job=%s(%d):%lu:%lu setrlimit(RLIMIT_DATA, ...) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id); - /* disable core files */ rlim.rlim_cur = 0; rlim.rlim_cur = 0; - - res = setrlimit(RLIMIT_CORE, &rlim); - if (res == -1) + if (setrlimit(RLIMIT_CORE, &rlim) == -1) mx_log_err("job=%s(%d):%lu:%lu setrlimit(RLIMIT_CORE, ...) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id); @@ -1067,37 +1046,30 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, rlim.rlim_cur = group->job_time*60; rlim.rlim_cur = group->job_time*63; - res = setrlimit(RLIMIT_CPU, &rlim); - if (res == -1) + if (setrlimit(RLIMIT_CPU, &rlim) == -1) mx_log_err("job=%s(%d):%lu:%lu setrlimit(RLIMIT_CPU, ...) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id); } - res = initgroups(passwd->pw_name, group->user_gid); - if (res == -1) { + if (initgroups(passwd->pw_name, group->user_gid) == -1) { mx_log_err("job=%s(%d):%lu:%lu initgroups() failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id); return; } - - res = setregid(group->user_gid, group->user_gid); - if (res == -1) { + if (setregid(group->user_gid, group->user_gid) == -1) { mx_log_err("job=%s(%d):%lu:%lu setregid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, group->user_gid, group->user_gid); return; } - - res = setreuid(-1, group->user_uid); - if (res == -1) { + if (setreuid(-1, group->user_uid) == -1) { mx_log_err("job=%s(%d):%lu:%lu setreuid(%d, %d) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, group->user_uid, group->user_uid); return; } - res = chdir(job->job_workdir); - if (res == -1) { + if (chdir(job->job_workdir) == -1) { mx_log_err("job=%s(%d):%lu:%lu chdir(%s) failed: %m", group->user_name, group->user_uid, group->group_id, job->job_id, job->job_workdir); @@ -1106,30 +1078,26 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, umask(job->job_umask); - res=sched_setaffinity(0,sizeof(job->host_cpu_set),&job->host_cpu_set); - if (res<0) mx_log_warning("sched_setaffinity: $m"); + if (sched_setaffinity(0,sizeof(job->host_cpu_set),&job->host_cpu_set) == -1) + mx_log_warning("sched_setaffinity: $m"); mxq_job_set_tmpfilenames(group, job); - res = mxq_redirect_input("/dev/null"); - if (res < 0) { - mx_log_err(" job=%s(%d):%lu:%lu mxq_redirect_input() failed (%d): %m", + if (mxq_redirect_input("/dev/null") < 0) { + mx_log_err(" job=%s(%d):%lu:%lu mxq_redirect_input() failed: %m", group->user_name, group->user_uid, group->group_id, - job->job_id, - res); + job->job_id); return; } - res = mxq_redirect_output(job->tmp_stdout, job->tmp_stderr); - if (res < 0) { - mx_log_err(" job=%s(%d):%lu:%lu mxq_redirect_output() failed (%d): %m", + if (mxq_redirect_output(job->tmp_stdout, job->tmp_stderr) < 0) { + mx_log_err(" job=%s(%d):%lu:%lu mxq_redirect_output() failed: %m", group->user_name, group->user_uid, group->group_id, - job->job_id, - res); + job->job_id); return; } @@ -1157,8 +1125,7 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, new_argv[i+4] = argv[i]; new_argv[argc+4] = NULL; - res = setuid(0); - if (res == -1) { + if (setuid(0) == -1) { mx_log_err("job=%s(%d):%lu:%lu setuid(0) failed: %m", group->user_name, group->user_uid, From 79ce67cdd6d2925f4c13dcb4518183092e1c1348 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 22:12:56 +0200 Subject: [PATCH 38/56] mxqd: Don't free() before exec() or _exit(). --- mxqd.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/mxqd.c b/mxqd.c index 1519a92e..23ee106d 100644 --- a/mxqd.c +++ b/mxqd.c @@ -997,7 +997,7 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, char *mxq_job_tmpdir = mx_asprintf_forever("%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job->job_id); mx_setenv_forever("MXQ_JOB_TMPDIR", mxq_job_tmpdir); mx_setenv_forever("TMPDIR", mxq_job_tmpdir); - free(mxq_job_tmpdir); + // not needed before exec() or exit(): free(mxq_job_tmpdir); } if (group->job_gpu) { char *argv[] = { @@ -1013,9 +1013,7 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, exit(1); } mx_setenv_forever("CUDA_VISIBLE_DEVICES", gpu_uuid); - free(gpu_uuid); - free(argv[2]); - free(argv[3]); + // not needed before exec() or exit(): free(gpu_uuid); free(argv[2]); free(argv[3]); } int fh = open("/proc/self/loginuid", O_WRONLY|O_TRUNC); @@ -1124,6 +1122,7 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, for (int i = 0; i < argc ; i++) new_argv[i+4] = argv[i]; new_argv[argc+4] = NULL; + // not needed before exec() or exit: free(argv); free(argv[1]); free(argv[2]); if (setuid(0) == -1) { mx_log_err("job=%s(%d):%lu:%lu setuid(0) failed: %m", @@ -1209,19 +1208,8 @@ static unsigned long start_job(struct mxq_group_list *glist) return 0; } else if (pid == 0) { job->host_pid = getpid(); - mx_log_debug("starting reaper process."); - mx_funlock_nodelete(server->flock); - server->flock = NULL; - mx_mysql_finish(&server->mysql); - exec_reaper(server, glist, job); - - mxq_job_free_content(job); - - mx_log_debug("shutting down reaper, bye bye."); - mx_log_finish(); - server_free(server); _exit(EX__MAX+1); } From 16aee8304a958dee7a372806837d72bdd2dd28eb Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 15:05:23 +0200 Subject: [PATCH 39/56] mx_util: Add close_range syscall wrapper for glibc < 2.34 --- mx_util.h | 5 +++++ test_mx_util.c | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/mx_util.h b/mx_util.h index 349e5717..173e7192 100644 --- a/mx_util.h +++ b/mx_util.h @@ -183,4 +183,9 @@ time_t mx_clock_boottime(void); int mx_call_external(char *helper, char **argv); char *mx_pipe_external(char *args, char **argv); +#if __GLIBC__ <2 || __GLIBC__ == 2 && __GLIBC_MINOR__ < 34 +#include +#define close_range(first, last, flags) syscall(SYS_close_range, first, last, flags) +#endif + #endif diff --git a/test_mx_util.c b/test_mx_util.c index 9ecf5343..236918fd 100644 --- a/test_mx_util.c +++ b/test_mx_util.c @@ -5,6 +5,8 @@ #include #include #include +#include +#include #include "mx_util.h" #include "mx_proc.h" @@ -589,6 +591,23 @@ static void test_mx_call_external(void) { assert(errno == EPROTO); } +static void test_mx_closerange(void) { + int res; + + int fd1 = open("/dev/null", O_RDONLY); + assert (fd1 != -1); + int fd2 = dup2(fd1, 100); + assert (fd2 == 100 ); + + res=close_range(3, ~0u, 0); + assert(res == 0); + + res = close(fd1); + assert (res == -1 && errno == EBADF); + res = close(fd2); + assert (res == -1 && errno == EBADF); +} + int main(void) { test_mx_strskipwhitespaces(); @@ -611,5 +630,6 @@ int main(void) test_mx_df(); test_mx_pipe_external(); test_mx_call_external(); + test_mx_closerange(); return 0; } From b065827cadf1f20f0c8346becca3bdef6c71a94f Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 22:15:35 +0200 Subject: [PATCH 40/56] mxqd: Do not disconnect from mysql server on job start Use close_range to set all file descriptors to close-on-exec to make sure that we don't leak the mysql socket (or any other) file descriptor to the user process. Now that we are sure, that the mysql socket is not leaked to the user and that the child doesn't call into mysql library code, because it will exec() or _exit(), there no longer is a need to disconnect from and reconnect to the mysql server every time we start a job, which is an expensive operation in mxqd and on the mysql server. Remove mx_mysql_disconnect before and mx_mysql_reconnect after the fork. --- mxqd.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mxqd.c b/mxqd.c index 23ee106d..e803103c 100644 --- a/mxqd.c +++ b/mxqd.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -1198,8 +1199,6 @@ static unsigned long start_job(struct mxq_group_list *glist) mx_free_null(job->host_cpu_set_str); job->host_cpu_set_str = mx_cpuset_to_str(&job->host_cpu_set); - mx_mysql_disconnect(server->mysql); - pid = fork(); if (pid < 0) { mx_log_err("fork: %m"); @@ -1209,14 +1208,17 @@ static unsigned long start_job(struct mxq_group_list *glist) } else if (pid == 0) { job->host_pid = getpid(); mx_log_debug("starting reaper process."); + // we would like to use CLOSE_RANGE_CLOEXEC, but would need Linux 5.11 for that + if (close_range(3, ~0u, 0) == -1) { + mx_log_fatal("close_range: %m"); + _exit(1); + } exec_reaper(server, glist, job); _exit(EX__MAX+1); } gettimeofday(&job->stats_starttime, NULL); - mx_mysql_connect_forever(&(server->mysql)); - job->host_pid = pid; job->host_slots = glist->slots_per_job; res = mxq_set_job_status_running(server->mysql, job); From d37ba3be9689cd2135d3546e5b5cec16c6e08ed0 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 22:33:34 +0200 Subject: [PATCH 41/56] helper/tmpdir-setup: Add cleanup command --- helper/tmpdir-setup | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/helper/tmpdir-setup b/helper/tmpdir-setup index aab193f4..088acdf3 100755 --- a/helper/tmpdir-setup +++ b/helper/tmpdir-setup @@ -4,6 +4,7 @@ usage() { cat <&2 usage: $0 create JOBID SIZE UID # create SIZE GiB /dev/shm/mxqd/mnt/job/$JOBID + $0 cleanup JOBID # cleanup EOF exit 1 } @@ -47,6 +48,16 @@ cmd_create() { exit $status } +cmd_cleanup() { + (( $# == 1 )) || usage + MXQ_JOBID=$1 + + mountpoint=$mntdir/$MXQ_JOBID + + umount $mountpoint + rmdir $mountpoint +} + (( $# > 0 )) || usage cmd="$1" shift; @@ -54,6 +65,9 @@ case "$cmd" in create) cmd_create "$@" ;; + cleanup) + cmd_cleanup "$@" + ;; *) usage ;; From 7b0d90c1485132c2f22f45e5241d8505710fad67 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Wed, 20 Apr 2022 22:44:47 +0200 Subject: [PATCH 42/56] mxqd: Use helper to unmount tmpdir Use the external helper to unmount the tmpdir. We are going to experiment with more complex tmpdir settings in the future which might be difficult to code in C. --- mxqd.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/mxqd.c b/mxqd.c index e803103c..746c2b9f 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1143,6 +1143,8 @@ static void exec_reaper(struct mxq_server *server,struct mxq_group_list *glist, argv[0]); } +static char tmpdir_script[] = LIBEXECDIR "/mxq/tmpdir-setup"; + static unsigned long start_job(struct mxq_group_list *glist) { struct mxq_server *server; @@ -1155,8 +1157,6 @@ static unsigned long start_job(struct mxq_group_list *glist) struct mxq_daemon *daemon; - static char tmpdir_script[] = LIBEXECDIR "/mxq/tmpdir-setup"; - pid_t pid; int res; @@ -1873,24 +1873,17 @@ static void rename_outfiles(struct mxq_server *server, struct mxq_group *group, } } -static int unmount_and_remove(char *pathname) { - int res; - res = rmdir(pathname); - if (res && errno==EBUSY) { - res = umount(pathname); - if (res == 0) { - res = rmdir(pathname); - } - } - return res; -} - static void unmount_job_tmpdir(unsigned long job_id) { - char *pathname = mx_asprintf_forever("%s/%lu", MXQ_JOB_TMPDIR_MNTDIR, job_id); - if (unmount_and_remove(pathname)) { - mx_log_warning("failed to unmount/remove stale job tmpdir %s: %m", pathname); - } - free(pathname); + char *argv[] = { + tmpdir_script, + "cleanup", + mx_asprintf_forever("%lu", job_id), + NULL + }; + int res = mx_call_external(tmpdir_script, argv); + free(argv[2]); + if (res == -1) + mx_log_err("cleanup job tmpdir: %m"); } static void release_gpu(struct mxq_server *server, struct mxq_group *group, struct mxq_job *job) { From c89786c8ca0f488dd3d3c2df4f0d5545b66d5c85 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 21 Apr 2022 07:45:29 +0200 Subject: [PATCH 43/56] mxqd: Don't let the child check status after fork --- mxqd.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/mxqd.c b/mxqd.c index 746c2b9f..0bc68891 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1200,12 +1200,7 @@ static unsigned long start_job(struct mxq_group_list *glist) job->host_cpu_set_str = mx_cpuset_to_str(&job->host_cpu_set); pid = fork(); - if (pid < 0) { - mx_log_err("fork: %m"); - cpuset_clear_running(&job->host_cpu_set,&server->cpu_set_available); - mxq_unload_job_from_server(server->mysql, job->job_id); - return 0; - } else if (pid == 0) { + if (pid == 0) { job->host_pid = getpid(); mx_log_debug("starting reaper process."); // we would like to use CLOSE_RANGE_CLOEXEC, but would need Linux 5.11 for that @@ -1216,6 +1211,12 @@ static unsigned long start_job(struct mxq_group_list *glist) exec_reaper(server, glist, job); _exit(EX__MAX+1); } + if (pid < 0) { + mx_log_err("fork: %m"); + cpuset_clear_running(&job->host_cpu_set,&server->cpu_set_available); + mxq_unload_job_from_server(server->mysql, job->job_id); + return 0; + } gettimeofday(&job->stats_starttime, NULL); From 05de58470203f8a0105fbc463a128a65017a7487 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Tue, 26 Apr 2022 09:54:59 +0200 Subject: [PATCH 44/56] mxqd: Remove missleading logging The number shown in the messages doesn't make any sense when we are running `mxqd --recover-only`, so remove the message. 2022-04-25 19:35:43 +0200 mxqd[122093]: recover: reload 9 running jobs from database 2022-04-25 19:35:43 +0200 mxqd[122093]: job finished (via fspool) : job 38218625 pid 79937 status 0 2022-04-25 19:35:43 +0200 mxqd[122093]: job finished (via fspool) : job 38315022 pid 41063 status 0 2022-04-25 19:35:49 +0200 mxqd[122093]: job finished (via fspool) : job 38357611 pid 21477 status 0 2022-04-25 19:35:49 +0200 mxqd[122093]: job finished (via fspool) : job 38359832 pid 52872 status 0 2022-04-25 19:35:49 +0200 mxqd[122093]: recover: processed 12008 finished jobs from fspool --- mxqd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/mxqd.c b/mxqd.c index 0bc68891..b2db6607 100644 --- a/mxqd.c +++ b/mxqd.c @@ -2444,8 +2444,9 @@ static int recover_from_previous_crash(struct mxq_server *server) mx_log_err("recover: server_fspool_scan: %m"); return res; } - if (res>0) - mx_log_info("recover: processed %d finished jobs from fspool",res); + /* Do not log slots returned, because this value is missleading with --recover-only as the current + * server may have much smaller slots then the previous server started with memory from + * mxqdctl-hostconfig */ res=lost_scan(server); if (res<0) { From ebf1a1a11ecc858aec7775a05b085b0509890c26 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 1 May 2022 18:54:33 +0200 Subject: [PATCH 45/56] mxqd.c: Remove two unused variables --- mxqd.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/mxqd.c b/mxqd.c index b2db6607..bfc4c86f 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1650,8 +1650,6 @@ static int killall(struct mxq_server *server) struct mxq_group_list *glist; struct mxq_job_list *jlist; - struct mxq_group *group; - struct ppidcache *ppidcache = ppidcache_new(); ppidcache_scan(ppidcache); @@ -1659,7 +1657,6 @@ static int killall(struct mxq_server *server) for (ulist = server->users; ulist; ulist = ulist->next) { for (glist = ulist->groups; glist; glist = glist->next) { - group = &glist->group; for (jlist = glist->jobs; jlist; jlist = jlist->next) killstate_event(ppidcache, jlist, KILLEVENT_CANCEL); } @@ -1783,9 +1780,7 @@ static int killall_cancelled(struct ppidcache *ppidcache, struct mxq_server *ser struct mxq_user_list *ulist; struct mxq_group_list *glist; struct mxq_job_list *jlist; - struct mxq_group *group; - struct mxq_job *job; assert(server); @@ -1801,7 +1796,6 @@ static int killall_cancelled(struct ppidcache *ppidcache, struct mxq_server *ser group->user_name, group->user_uid, group->group_id); for (jlist = glist->jobs; jlist; jlist = jlist->next) { - job = &jlist->job; killstate_event(ppidcache, jlist, KILLEVENT_CANCEL); } } From fc872f1034a594318c40b516fc5412943f911bde Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 1 May 2022 18:54:54 +0200 Subject: [PATCH 46/56] Makefile: Remove two obsolete warning exceptions --- Makefile | 2 -- 1 file changed, 2 deletions(-) diff --git a/Makefile b/Makefile index 2f7a6c0f..7a596264 100644 --- a/Makefile +++ b/Makefile @@ -499,7 +499,6 @@ mxqd.o: CFLAGS += $(CFLAGS_MYSQL) mxqd.o: CFLAGS += $(CFLAGS_MXQ_INITIAL_PATH) mxqd.o: CFLAGS += $(CFLAGS_MXQ_INITIAL_TMPDIR) mxqd.o: CFLAGS += $(CFLAGS_MXQ_FINISHED_JOBSDIR) -mxqd.o: CFLAGS += -Wno-unused-but-set-variable clean: CLEAN += mxqd.o @@ -587,7 +586,6 @@ mxqdump: mxq_job.o mxqdump: mx_util.o mxqdump: mx_getopt.o mxqdump: LDLIBS += $(LDLIBS_MYSQL) -mxqdump: CFLAGS += -Wunused-function build: mxqdump From 567844d6b0c61d4f610bb1a71f5a9aa30b009be9 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 1 May 2022 20:32:52 +0200 Subject: [PATCH 47/56] Revert "mx_util: Avoid false maybe-uninitialized warnings" This reverts commit 83531e851cda6122864ef7e7876f947fb2dd483f. There will be another solution in the following commit. --- mx_util.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/mx_util.c b/mx_util.c index d04d21dc..48718e56 100644 --- a/mx_util.c +++ b/mx_util.c @@ -356,7 +356,7 @@ int mx_strtoll(char *str, signed long long int *to) int mx_strtoui(char *str, unsigned int *to) { - unsigned long int ul = 0; /* avoid false maybe-uninitialized warning */ + unsigned long int ul; int res; assert(str); @@ -376,7 +376,7 @@ int mx_strtoui(char *str, unsigned int *to) int mx_strtou8(char *str, uint8_t *to) { - unsigned long int ul = 0; /* avoid false maybe-uninitialized warning */ + unsigned long int ul; int res; assert(str); @@ -396,7 +396,7 @@ int mx_strtou8(char *str, uint8_t *to) int mx_strtou16(char *str, uint16_t *to) { - unsigned long int ul = 0; /* avoid false maybe-uninitialized warning */ + unsigned long int ul; int res; assert(str); @@ -416,7 +416,7 @@ int mx_strtou16(char *str, uint16_t *to) int mx_strtou32(char *str, uint32_t *to) { - unsigned long int ul = 0; /* avoid false maybe-uninitialized warning */ + unsigned long int ul; int res; assert(str); @@ -436,7 +436,7 @@ int mx_strtou32(char *str, uint32_t *to) int mx_strtou64(char *str, uint64_t *to) { - unsigned long long int ull = 0; /* avoid false maybe-uninitialized warning */; + unsigned long long int ull; int res; assert(str); @@ -458,7 +458,7 @@ int mx_strtou64(char *str, uint64_t *to) int mx_strtoi(char *str, signed int *to) { - signed long int l = 0; /* avoid false maybe-uninitialized warning */ + signed long int l; int res; assert(str); @@ -478,7 +478,7 @@ int mx_strtoi(char *str, signed int *to) int mx_strtoi8(char *str, int8_t *to) { - signed long int l = 0; /* avoid false maybe-uninitialized warning */ + signed long int l; int res; assert(str); @@ -498,7 +498,7 @@ int mx_strtoi8(char *str, int8_t *to) int mx_strtoi16(char *str, int16_t *to) { - signed long int l = 0; /* avoid false maybe-uninitialized warning */ + signed long int l; int res; assert(str); @@ -518,7 +518,7 @@ int mx_strtoi16(char *str, int16_t *to) int mx_strtoi32(char *str, int32_t *to) { - signed long int l = 0; /* avoid false maybe-uninitialized warning */ + signed long int l; int res; assert(str); @@ -538,7 +538,7 @@ int mx_strtoi32(char *str, int32_t *to) int mx_strtoi64(char *str, int64_t *to) { - signed long long int ll = 0; /* avoid false maybe-uninitialized warning */ + signed long long int ll; int res; assert(str); @@ -796,7 +796,7 @@ int mx_read_first_line_from_file(char *fname, char **line) int mx_strscan_ull(char **str, unsigned long long int *to) { - unsigned long long int l = 0; /* avoid false maybe-uninitialized warning */; + unsigned long long int l; char *s; char *p; char o = 0; @@ -827,7 +827,7 @@ int mx_strscan_ull(char **str, unsigned long long int *to) int mx_strscan_ll(char **str, long long int *to) { - long long int l = 0; /* avoid false maybe-uninitialized warning */; + long long int l; char *s; char *p; char o = 0; From 0663c4e9200ecd06ca79eb49d7b2c94b0484433c Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 1 May 2022 20:48:35 +0200 Subject: [PATCH 48/56] mx_util: Avoid "may be used uninitialized" warning The function int mx_strtoul(char *str, unsigned long int *to) and three similar functions either returns with the success value 0 or with a negative error code (-errno). The output argument `to` is only written, when the return value signals success. If the function returns with a negative value, the caller can not assument that `to` has been set. When a caller does something like this int value; res = int mx_strtoul(str, &value); if (res < 0) return; do_something_with(value) everything is correct according to the description aboce. The code in mx_strtoul has this pattern: errno = 0; ul = strtoul(str, &end, 0); if (errno) { return -errno; } *to = ul; When the caller is in the same compilation unit, the compiler (gcc with -O2 or higher and with -Wall) might raise a "may be used uninitialized" warning for the output variable (`value` in the above example). The problem here is, that the caller assumes a negative value for an error status but the callee returns -errno for any errno value other than zero which might include negative errno values. We know, that the errno value will not be negative because we've set it to zero and the library function will conditionally set it to a valid error number only, which is alway positiv[1]. But the compiler doesn't share this assumption and takes a negative errno value into account. When the caller and the callee are in the same compilation unit, the interprocedural optimizer might detect the code path with a negative errno value which would in fact trigger a usage of an unitilialized value. We can fix that by just baking the assumption into the comparison: if (errno > 0) return -errno; or by asserting that errno is not negative: if (errno) { assert(errno > 0); return -errno; } or by defining and using an __assume macro: #if defined(__clang__) #define assume(cond) __builtin_assume(cond) #elif defined(__GNUC__) #define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0) #endif ... if (errno) { assume(errno > 0); return -errno; } Use the first option for all four affected functions. Note, that clangs static analyzer is still not happy with this, but this is probably a false positive of the analyzer [2]. [1]: errno(3): "Valid error numbers are all positive numbers. [2]: https://github.com/llvm/llvm-project/issues/55241 --- mx_util.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mx_util.c b/mx_util.c index 48718e56..bc472ab0 100644 --- a/mx_util.c +++ b/mx_util.c @@ -256,7 +256,7 @@ int mx_strtoul(char *str, unsigned long int *to) ul = strtoul(str, &end, 0); - if (errno) + if (errno > 0) return -errno; end = mx_strskipwhitespaces(end); @@ -284,7 +284,7 @@ int mx_strtoull(char *str, unsigned long long int *to) ull = strtoull(str, &end, 0); - if (errno) + if (errno > 0) return -errno; end = mx_strskipwhitespaces(end); @@ -314,7 +314,7 @@ int mx_strtol(char *str, signed long int *to) l = strtoul(str, &end, 0); - if (errno) + if (errno > 0) return -errno; end = mx_strskipwhitespaces(end); @@ -339,7 +339,7 @@ int mx_strtoll(char *str, signed long long int *to) ll = strtoll(str, &end, 0); - if (errno) + if (errno > 0) return -errno; end = mx_strskipwhitespaces(end); From 3763a09c34a30f89b27dedb28caaaac0f18ab770 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Mon, 2 May 2022 08:01:11 +0200 Subject: [PATCH 49/56] mxqd: Remove unused "pagesize" value --- mxqd.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/mxqd.c b/mxqd.c index bfc4c86f..c54e0369 100644 --- a/mxqd.c +++ b/mxqd.c @@ -1720,7 +1720,6 @@ static int killall_over_memory(struct ppidcache *ppidcache, struct mxq_server *s struct mx_proc_tree *ptree = NULL; struct mx_proc_info *pinfo; - long pagesize; int res; assert(server); @@ -1731,12 +1730,6 @@ static int killall_over_memory(struct ppidcache *ppidcache, struct mxq_server *s /* limit killing to every >= 10 seconds */ mx_within_rate_limit_or_return(10, 0); - pagesize = sysconf(_SC_PAGESIZE); - if (!pagesize) { - mx_log_warning("killall_over_memory(): Can't get _SC_PAGESIZE. Assuming 4096."); - pagesize = 4096; - } - res = mx_proc_tree(&ptree); if (res < 0) { mx_log_err("killall_over_memory(): Reading process tree failed: %m"); From ca8919653ad21928dcba12574d2bfbbe84e77adb Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Mon, 2 May 2022 08:05:37 +0200 Subject: [PATCH 50/56] mxqd: Remove dead store --- mxqd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mxqd.c b/mxqd.c index c54e0369..9a50497b 100644 --- a/mxqd.c +++ b/mxqd.c @@ -2705,7 +2705,7 @@ int main(int argc, char *argv[]) mx_log_info("-------------------------------------------------------------"); mx_log_info(" Reexecuting %s", argv[0]); mx_log_info("-------------------------------------------------------------"); - res = execvp(argv[0], argv); + execvp(argv[0], argv); mx_log_fatal("execvp(\"%s\", ...): %m", argv[0]); } From cf0f12260574a665b6f1fe6a7eb02aebad1e174f Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Mon, 2 May 2022 08:12:36 +0200 Subject: [PATCH 51/56] mxqsub: Remove dead store --- mxqsub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mxqsub.c b/mxqsub.c index 802d13b2..7a870587 100644 --- a/mxqsub.c +++ b/mxqsub.c @@ -560,7 +560,7 @@ static int add_job(struct mx_mysql *mysql, struct mxq_job *j) j->job_id = insert_id; - res = mx_mysql_statement_close(&stmt); + mx_mysql_statement_close(&stmt); return (int)num_rows; } From 49442f97bf072014d01b5bf60b54edcc239bf75f Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Tue, 3 May 2022 11:20:00 +0200 Subject: [PATCH 52/56] mx_mxql: Initialize s->func When we run into a (very unlikely) early error condition, the error macros might output s->func. While glibc printf functions output "(null)" if NULL is passed to "%s", this is not guaranteed. For example, gcc transforms `printf("%s\n", ptr)` into `puts(ptr)`. --- mx_mysql.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mx_mysql.c b/mx_mysql.c index aff7bc02..87e4789b 100644 --- a/mx_mysql.c +++ b/mx_mysql.c @@ -945,6 +945,7 @@ static int mx_mysql_statement_init(struct mx_mysql *mysql, struct mx_mysql_stmt s = mx_calloc_forever(1, sizeof(*s)); s->mysql = mysql; + s->func = ""; do { res = mx__mysql_stmt_init(s); From 3f898695216f2c713a29ff409fdf3abfe3a34b51 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 14 Apr 2022 21:07:11 +0200 Subject: [PATCH 53/56] Makefile: Enable -Wextra Enable -Wextra. Exclude -Wno-override-init for now, because mx_getopt.h relies on it [1]. [1]: https://github.molgen.mpg.de/mariux64/mxq/issues/131 --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 7a596264..0cbc77db 100644 --- a/Makefile +++ b/Makefile @@ -122,7 +122,7 @@ CFLAGS_MYSQL += -DMX_MYSQL_FAIL_WAIT_DEFAULT=5 CFLAGS += -g CFLAGS += -O3 -CFLAGS += -Wall +CFLAGS += -Wall -Wextra -Wno-override-init CFLAGS += -DMXQ_VERSION=\"${MXQ_VERSION}\" CFLAGS += -DMXQ_VERSIONFULL=\"${MXQ_VERSIONFULL}\" CFLAGS += -DMXQ_VERSIONDATE=\"${MXQ_VERSIONDATE}\" From 16b0457ba6de16eb8328aec4d03a76e5bb19df61 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Mon, 2 May 2022 08:03:46 +0200 Subject: [PATCH 54/56] Makefile: Add target to run LLVMs static analyzer --- Makefile | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Makefile b/Makefile index 0cbc77db..8fab4f6b 100644 --- a/Makefile +++ b/Makefile @@ -289,6 +289,13 @@ install:: ######################################################################## +.PHONY: scan-build + +scan-build:: + scan-build $(MAKE) build + +######################################################################## + ### mx_log.h ----------------------------------------------------------- mx_log.h += mx_log.h From 9640f65abf69851d1124ec9866c4ea6a3001b266 Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Thu, 5 May 2022 20:42:01 +0200 Subject: [PATCH 55/56] tmpdir-setup: Make umount faster The umount of the job tmpdir often takes a lot of time (minutes!) when the user write a lot of data to it, because there are many dirty pages which are written to the disk before the umount completes. This writing is obviously a waste of resources, because as soon as the unmount is finished, the backup image for the filesystem is destroyed anyway. Unfortunatly, we currenty don't have a solid way to avoid the unnecessary writeback. What we can do is to purge the directory. Experiments show, that this is indeed faster with both: few very big files or many small files. So do that. Hard-code the directory prefix into the rm command to decrease the risk of rm removing wrong files in case future code changes are buggy and, for example, missspell a shell variable. Because the cleanup still needs time, do that in the background so that mxqd can continue. --- helper/tmpdir-setup | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/helper/tmpdir-setup b/helper/tmpdir-setup index 088acdf3..fe47dc79 100755 --- a/helper/tmpdir-setup +++ b/helper/tmpdir-setup @@ -52,10 +52,12 @@ cmd_cleanup() { (( $# == 1 )) || usage MXQ_JOBID=$1 - mountpoint=$mntdir/$MXQ_JOBID - - umount $mountpoint - rmdir $mountpoint + ( + shopt -s dotglob; + rm -rf /dev/shm/mxqd/mnt/job/$MXQ_JOBID/* + umount /dev/shm/mxqd/mnt/job/$MXQ_JOBID + rmdir /dev/shm/mxqd/mnt/job/$MXQ_JOBID + ) & } (( $# > 0 )) || usage From efe43f4df38ec9bf9d6c1f0863a2476ae5045f9d Mon Sep 17 00:00:00 2001 From: Donald Buczek Date: Sun, 8 May 2022 20:01:20 +0200 Subject: [PATCH 56/56] mxqsub: Default tmpdir to 100G --- mxqsub.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mxqsub.c b/mxqsub.c index 7a870587..7ed7e69a 100644 --- a/mxqsub.c +++ b/mxqsub.c @@ -67,7 +67,7 @@ static void print_usage(void) "\n" " -j, --processors=NUMBER set number of processors (default: 1)\n" " -m, --memory=SIZE set amount of memory (default: 2G)\n" - " --tmpdir=SIZE set size of MXQ_JOB_TMPDIR (default: 0)\n" + " --tmpdir=SIZE set size of MXQ_JOB_TMPDIR (default: 100G)\n" " --gpu request a gpu\n" " --blacklist=STRING set list of blacklisted servers (default: '')\n" " --whitelist=STRING set list of whitelisted servers (default: '')\n" @@ -803,7 +803,7 @@ int main(int argc, char *argv[]) arg_debug = 0; arg_jobflags = 0; arg_groupid = UINT64_UNSET; - arg_tmpdir = 0; + arg_tmpdir = 100; // 100G arg_blacklist = NULL; arg_whitelist = NULL; arg_prerequisites = "";