From: Andreas Dilger Date: Fri, 10 Aug 2012 22:24:00 +0000 (-0600) Subject: LU-1735 ptlrpc: cleanup jobid code X-Git-Tag: 2.3.51~139 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=f6085c49dfb866934d4ba62b7e2a328102ed8a83 LU-1735 ptlrpc: cleanup jobid code Some cleanups for the jobid code: - if obd_jobid_var is too large, only print an error message once - in linux cfs_get_environ(): -- cache strlen(key) since it doesn't change and is used often -- remove unnecessary typecasts of void pointers -- use "rc" instead of "ret" -- balance ENTRY and RETURN/GOTO calls - add cfs_get_environ() for liblustre and remove inline #ifdef - use strcmp() to compare strings that are known NUL-terminated - use strlcpy() to ensure NUL-terminated strings in target buffer -- add strlcpy() wrapper for liblustre, it isn't in Glibc on RHEL5 Signed-off-by: Andreas Dilger Change-Id: I22c3d5c1755c1d6aab666a769df38218b954cab0 Reviewed-on: http://review.whamcloud.com/3713 Tested-by: Hudson Reviewed-by: Niu Yawei Reviewed-by: Jinshan Xiong Tested-by: Maloo Reviewed-by: Oleg Drokin --- diff --git a/libcfs/autoconf/lustre-libcfs.m4 b/libcfs/autoconf/lustre-libcfs.m4 index da15da9..28663b6 100644 --- a/libcfs/autoconf/lustre-libcfs.m4 +++ b/libcfs/autoconf/lustre-libcfs.m4 @@ -835,6 +835,9 @@ AC_CHECK_TYPE([spinlock_t], # lnet/utils/wirecheck.c AC_CHECK_FUNCS([strnlen]) +# lnet/libcfs/user-prim.c, missing for RHEL5 and earlier userspace +AC_CHECK_FUNCS([strlcpy]) + AC_CHECK_TYPE([umode_t], [AC_DEFINE(HAVE_UMODE_T, 1, [umode_t is defined])], [], diff --git a/libcfs/include/libcfs/curproc.h b/libcfs/include/libcfs/curproc.h index d5530fc..17c12a1 100644 --- a/libcfs/include/libcfs/curproc.h +++ b/libcfs/include/libcfs/curproc.h @@ -47,12 +47,8 @@ * * Implemented in portals/include/libcfs// */ -uid_t cfs_curproc_uid(void); -gid_t cfs_curproc_gid(void); uid_t cfs_curproc_euid(void); gid_t cfs_curproc_egid(void); -uid_t cfs_curproc_fsuid(void); -gid_t cfs_curproc_fsgid(void); pid_t cfs_curproc_pid(void); int cfs_curproc_groups_nr(void); int cfs_curproc_is_in_groups(gid_t group); @@ -73,8 +69,12 @@ char *cfs_curproc_comm(void); /* check if task is running in compat mode.*/ int cfs_curproc_is_32bit(void); -int cfs_get_environ(const char *key, char *value, int *val_len); #endif +uid_t cfs_curproc_uid(void); +gid_t cfs_curproc_gid(void); +uid_t cfs_curproc_fsuid(void); +gid_t cfs_curproc_fsgid(void); +int cfs_get_environ(const char *key, char *value, int *val_len); typedef __u32 cfs_cap_t; diff --git a/libcfs/include/libcfs/user-prim.h b/libcfs/include/libcfs/user-prim.h index 76e6c33..c8e5611 100644 --- a/libcfs/include/libcfs/user-prim.h +++ b/libcfs/include/libcfs/user-prim.h @@ -163,6 +163,10 @@ gid_t cfs_curproc_gid(void); uid_t cfs_curproc_fsuid(void); gid_t cfs_curproc_fsgid(void); +#ifndef HAVE_STRLCPY /* not in glibc for RHEL 5.x, remove when obsolete */ +size_t strlcpy(char *tgt, const char *src, size_t tgt_len); +#endif + #define LIBCFS_REALLOC(ptr, size) realloc(ptr, size) #define cfs_online_cpus() sysconf(_SC_NPROCESSORS_ONLN) diff --git a/libcfs/libcfs/linux/linux-curproc.c b/libcfs/libcfs/linux/linux-curproc.c index c1e814d..b032ec3 100644 --- a/libcfs/libcfs/linux/linux-curproc.c +++ b/libcfs/libcfs/linux/linux-curproc.c @@ -233,12 +233,12 @@ static int cfs_access_process_vm(struct task_struct *tsk, unsigned long addr, down_read(&mm->mmap_sem); /* ignore errors, just check how much was sucessfully transfered */ while (len) { - int bytes, ret, offset; + int bytes, rc, offset; void *maddr; - ret = get_user_pages(tsk, mm, addr, 1, + rc = get_user_pages(tsk, mm, addr, 1, write, 1, &page, &vma); - if (ret <= 0) + if (rc <= 0) break; bytes = len; @@ -274,17 +274,18 @@ int cfs_get_environ(const char *key, char *value, int *val_len) struct mm_struct *mm; char *buffer, *tmp_buf = NULL; int buf_len = CFS_PAGE_SIZE; + int key_len = strlen(key); unsigned long addr; - int ret; + int rc; ENTRY; - buffer = (char *)cfs_alloc(buf_len, CFS_ALLOC_USER); + buffer = cfs_alloc(buf_len, CFS_ALLOC_USER); if (!buffer) RETURN(-ENOMEM); mm = get_task_mm(current); if (!mm) { - cfs_free((void *)buffer); + cfs_free(buffer); RETURN(-EINVAL); } @@ -297,15 +298,13 @@ int cfs_get_environ(const char *key, char *value, int *val_len) up_read(&mm->mmap_sem); addr = mm->env_start; - ret = -ENOENT; - while (addr < mm->env_end) { int this_len, retval, scan_len; char *env_start, *env_end; memset(buffer, 0, buf_len); - this_len = min((int)(mm->env_end - addr), buf_len); + this_len = min_t(int, mm->env_end - addr, buf_len); retval = cfs_access_process_vm(current, addr, buffer, this_len, 0); if (retval != this_len) @@ -321,7 +320,7 @@ int cfs_get_environ(const char *key, char *value, int *val_len) char *entry; int entry_len; - env_end = (char *)memscan(env_start, '\0', scan_len); + env_end = memscan(env_start, '\0', scan_len); LASSERT(env_end >= env_start && env_end <= env_start + scan_len); @@ -331,8 +330,7 @@ int cfs_get_environ(const char *key, char *value, int *val_len) /* This entry is too large to fit in buffer */ if (unlikely(scan_len == this_len)) { CERROR("Too long env variable.\n"); - ret = -EINVAL; - goto out; + GOTO(out, rc = -EINVAL); } addr -= scan_len; break; @@ -342,34 +340,31 @@ int cfs_get_environ(const char *key, char *value, int *val_len) entry_len = env_end - env_start; /* Key length + length of '=' */ - if (entry_len > strlen(key) + 1 && - !memcmp(entry, key, strlen(key))) { - entry += (strlen(key) + 1); - entry_len -= (strlen(key) + 1); + if (entry_len > key_len + 1 && + !memcmp(entry, key, key_len)) { + entry += key_len + 1; + entry_len -= key_len + 1; /* The 'value' buffer passed in is too small.*/ - if (entry_len >= *val_len) { - CERROR("Buffer is too small. " - "entry_len=%d buffer_len=%d\n", - entry_len, *val_len); - ret = -EOVERFLOW; - } else { - memcpy(value, entry, entry_len); - *val_len = entry_len; - ret = 0; - } - goto out; + if (entry_len >= *val_len) + GOTO(out, rc = -EOVERFLOW); + + memcpy(value, entry, entry_len); + *val_len = entry_len; + GOTO(out, rc = 0); } scan_len -= (env_end - env_start + 1); env_start = env_end + 1; } } + GOTO(out, rc = -ENOENT); + out: mmput(mm); cfs_free((void *)buffer); if (tmp_buf) cfs_free((void *)tmp_buf); - RETURN(ret); + return rc; } EXPORT_SYMBOL(cfs_get_environ); diff --git a/libcfs/libcfs/user-prim.c b/libcfs/libcfs/user-prim.c index e6aaaea..0d8afc6 100644 --- a/libcfs/libcfs/user-prim.c +++ b/libcfs/libcfs/user-prim.c @@ -46,6 +46,7 @@ #ifndef __KERNEL__ +#include #include /* @@ -263,6 +264,35 @@ gid_t cfs_curproc_fsgid(void) return getgid(); } +#ifndef HAVE_STRLCPY /* not in glibc for RHEL 5.x, remove when obsolete */ +size_t strlcpy(char *tgt, const char *src, size_t tgt_len) +{ + int src_len = strlen(src); + + strncpy(tgt, src, tgt_len - 1); + tgt[tgt_len - 1] = '\0'; + + return src_len + 1; +} +#endif + +/* Read the environment variable of current process specified by @key. */ +int cfs_get_environ(const char *key, char *value, int *val_len) +{ + char *entry; + int len; + + entry = getenv(key); + if (entry == NULL) + return -ENOENT; + + len = strlcpy(value, entry, *val_len); + if (len >= *val_len) + return -EOVERFLOW; + + return 0; +} + void cfs_enter_debugger(void) { /* diff --git a/lustre/obdclass/class_obd.c b/lustre/obdclass/class_obd.c index 1e935f4..7e60d09 100644 --- a/lustre/obdclass/class_obd.c +++ b/lustre/obdclass/class_obd.c @@ -118,36 +118,47 @@ EXPORT_SYMBOL(obd_jobid_var); */ int lustre_get_jobid(char *jobid) { -#ifdef __KERNEL__ int jobid_len = JOBSTATS_JOBID_SIZE; -#endif - int ret = 0; + int rc = 0; ENTRY; memset(jobid, 0, JOBSTATS_JOBID_SIZE); /* Jobstats isn't enabled */ - if (!memcmp(obd_jobid_var, JOBSTATS_DISABLE, - strlen(JOBSTATS_DISABLE))) { + if (strcmp(obd_jobid_var, JOBSTATS_DISABLE) == 0) RETURN(0); - } /* Use process name + fsuid as jobid */ - if (!memcmp(obd_jobid_var, JOBSTATS_PROCNAME_UID, - strlen(JOBSTATS_PROCNAME_UID))) { + if (strcmp(obd_jobid_var, JOBSTATS_PROCNAME_UID) == 0) { snprintf(jobid, JOBSTATS_JOBID_SIZE, "%s_%u", cfs_curproc_comm(), cfs_curproc_fsuid()); RETURN(0); } -#ifdef __KERNEL__ - ret = cfs_get_environ(obd_jobid_var, jobid, &jobid_len); - if (ret) { - CDEBUG((ret != -ENOENT && ret != -EINVAL && ret != -EDEADLK) ? - D_ERROR : D_INFO, "Get jobid for (%s) failed: rc = %d\n", - obd_jobid_var, ret); + rc = cfs_get_environ(obd_jobid_var, jobid, &jobid_len); + if (rc) { + if (rc == -EOVERFLOW) { + /* For the PBS_JOBID and LOADL_STEP_ID keys (which are + * variable length strings instead of just numbers), it + * might make sense to keep the unique parts for JobID, + * instead of just returning an error. That means a + * larger temp buffer for cfs_get_environ(), then + * truncating the string at some separator to fit into + * the specified jobid_len. Fix later if needed. */ + static bool printed; + if (unlikely(!printed)) { + LCONSOLE_ERROR_MSG(0x16b, "%s value too large " + "for JobID buffer (%d)\n", + obd_jobid_var, jobid_len); + printed = true; + } + } else { + CDEBUG((rc == -ENOENT && rc == -EINVAL && + rc == -EDEADLK) ? D_INFO : D_ERROR, + "Get jobid for (%s) failed: rc = %d\n", + obd_jobid_var, rc); + } } -#endif - RETURN(ret); + RETURN(rc); } EXPORT_SYMBOL(lustre_get_jobid); diff --git a/lustre/obdclass/obd_config.c b/lustre/obdclass/obd_config.c index da9734e..7d82c5b 100644 --- a/lustre/obdclass/obd_config.c +++ b/lustre/obdclass/obd_config.c @@ -44,6 +44,7 @@ #include #else #include +#include #include #include #endif @@ -899,11 +900,10 @@ static int class_set_global(char *ptr, int val, struct lustre_cfg *lcfg) at_early_margin = val; else if (class_match_param(ptr, PARAM_AT_HISTORY, NULL) == 0) at_history = val; - else if (class_match_param(ptr, PARAM_JOBID_VAR, NULL) == 0) { - memset(obd_jobid_var, 0, JOBSTATS_JOBID_VAR_MAX_LEN + 1); - memcpy(obd_jobid_var, lustre_cfg_string(lcfg, 2), - JOBSTATS_JOBID_VAR_MAX_LEN + 1); - } else + else if (class_match_param(ptr, PARAM_JOBID_VAR, NULL) == 0) + strlcpy(obd_jobid_var, lustre_cfg_string(lcfg, 2), + JOBSTATS_JOBID_VAR_MAX_LEN + 1); + else RETURN(-EINVAL); CDEBUG(D_IOCTL, "global %s = %d\n", ptr, val);