Whamcloud - gitweb
LU-1735 ptlrpc: cleanup jobid code
authorAndreas Dilger <adilger@whamcloud.com>
Fri, 10 Aug 2012 22:24:00 +0000 (16:24 -0600)
committerOleg Drokin <green@whamcloud.com>
Thu, 6 Sep 2012 14:29:08 +0000 (10:29 -0400)
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 <adilger@whamcloud.com>
Change-Id: I22c3d5c1755c1d6aab666a769df38218b954cab0
Reviewed-on: http://review.whamcloud.com/3713
Tested-by: Hudson
Reviewed-by: Niu Yawei <niu@whamcloud.com>
Reviewed-by: Jinshan Xiong <jinshan.xiong@whamcloud.com>
Tested-by: Maloo <whamcloud.maloo@gmail.com>
Reviewed-by: Oleg Drokin <green@whamcloud.com>
libcfs/autoconf/lustre-libcfs.m4
libcfs/include/libcfs/curproc.h
libcfs/include/libcfs/user-prim.h
libcfs/libcfs/linux/linux-curproc.c
libcfs/libcfs/user-prim.c
lustre/obdclass/class_obd.c
lustre/obdclass/obd_config.c

index da15da9..28663b6 100644 (file)
@@ -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])],
        [],
index d5530fc..17c12a1 100644 (file)
  *
  * Implemented in portals/include/libcfs/<os>/
  */
-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;
 
index 76e6c33..c8e5611 100644 (file)
@@ -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)
index c1e814d..b032ec3 100644 (file)
@@ -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);
 
index e6aaaea..0d8afc6 100644 (file)
@@ -46,6 +46,7 @@
 
 #ifndef __KERNEL__
 
+#include <string.h>
 #include <libcfs/libcfs.h>
 
 /*
@@ -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)
 {
         /*
index 1e935f4..7e60d09 100644 (file)
@@ -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);
 
index da9734e..7d82c5b 100644 (file)
@@ -44,6 +44,7 @@
 #include <linux/string.h>
 #else
 #include <liblustre.h>
+#include <string.h>
 #include <obd_class.h>
 #include <obd.h>
 #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);