Re: [PATCH][PR threads/15824] when get threads name failed from info threads with linux kernel version earlier than 2.6.33

[Yao Qi <yao at codesourcery dot com>]

Hi Will,
I am not the 'right' person to review this patch, however, I find some
thing obviously incorrect.  I point them out here.

On 08/14/2013 12:26 PM, Will Huang wrote:
> diff -ruN ./gdb.org/ChangeLog ./gdb/ChangeLog
> --- ./gdb.org/ChangeLog 2013-04-26 19:43:30.000000000 +0800
> +++ ./gdb/ChangeLog     2013-08-14 11:09:08.000000000 +0800
> @@ -1,3 +1,11 @@
> +2013-08-14  Will Huang<will.huang@aliyun-inc.com>
> +
> +       PR threads/15824

Each line should be prefixed with 'TAB' instead of 8 spaces.

> +       * linux-nat.c (linux_nat_thread_name): Add get thread name from
> +       procfs "/proc/$pid/task/$tid/status".
> +       * commo/linux-procfs.c (linux_proc_get_thread_name): New.
> +       * commo/linux-procfs.c (linux_proc_get_string): New.
          ^^^^^^^^^^^^^^^^^^^^^^^  Duplicated.

Only have to mention the same file once.

> +
>   2013-04-26  Joel Brobecker<brobecker@adacore.com>
>
>          * NEWS: Change "since GDB 7.5" into "in GDB 7.6".
> diff -ruN ./gdb.org/common/linux-procfs.c ./gdb/common/linux-procfs.c
> --- ./gdb.org/common/linux-procfs.c     2013-01-01 14:32:54.000000000 +0800
> +++ ./gdb/common/linux-procfs.c 2013-08-12 16:51:33.000000000 +0800
> @@ -55,6 +55,51 @@
>     return retval;
>   }
>
> +/* Return the string length of Name from/proc/$pid/task/$tid/status.

Space after "from" is needed.
s/Name/FIELD
s/$pid/LWPID
s/$tid/TID

> +   Returns -1 if not found.  */

s/Returns/Return

> +
> +static int
> +linux_proc_get_string (pid_t lwpid, pid_t tid,
> +                       char *target, size_t t_size, const char *field)

"char *target" should go to the previous line.

> +{
> +  size_t field_len = strlen (field);
> +  FILE *status_file;
> +  char buf[100];
> +  int retval = -1;
> +
> +  if (tid > 0)
> +       snprintf (buf, sizeof (buf),
> +                  "/proc/%d/task/%d/status", (int) lwpid, (int) tid);

Use xsnprintf, and the indentation looks wrong.

> +  else
> +       snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid);
> +

Likewise.  Is it a dead path?  Can 'tid' be negative?

> +  status_file = fopen (buf, "r");

Use gdb_fopen_cloexec?

> +  if (status_file == NULL)
> +    {
> +      warning (_("unable to open /proc file '%s'"), buf);
> +      return -1;
> +    }
> +
> +  while (fgets (buf, sizeof (buf), status_file))
> +    if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':')
> +      {
> +       size_t pos = field_len + 1;
> +       while((buf[pos] == ' ' || buf[pos] == '\t') && (pos < sizeof (buf) -
> 1))

Your mailer wrap this line...

> +         pos++;
> +
> +       strncpy (target, &buf[pos], t_size);
> +       target[t_size - 1] = '\0';
> +       retval = strlen (target);
> +       if(retval > 0 && target[retval - 1] == '\n')
> +               target[retval - 1] = '\0';
> +
> +       break;
> +      }
> +
> +  fclose (status_file);
> +  return retval;
> +}
> +
>   /* Return the TGID of LWPID from /proc/pid/status.  Returns -1 if not
>      found.  */
>
> @@ -118,3 +163,11 @@
>   {
>     return linux_proc_pid_has_state (pid, "Z (zombie)");
>   }
> +
> +/*Get thread name identified by PID & TID*/

I am not good at comments either, but I suggest

/* Save the name of thread PID/TID into buffer NAME.  SIZE is the size
   of buffer NAME.  Return the length of the name if success,
   otherwise return -1.  */

> +
> +int
> +linux_proc_get_thread_name (pid_t pid, pid_t tid, char *name, size_t size)
> +{
> +  return linux_proc_get_string (pid, tid, name, size, "Name");
> +}
> diff -ruN ./gdb.org/common/linux-procfs.h ./gdb/common/linux-procfs.h
> --- ./gdb.org/common/linux-procfs.h     2013-01-01 14:32:54.000000000 +0800
> +++ ./gdb/common/linux-procfs.h 2013-08-12 16:50:39.000000000 +0800
> @@ -40,4 +40,10 @@
>
>   extern int linux_proc_pid_is_zombie (pid_t pid);
>
> +/* Return -1 if thread Name of tid didn't found;

Comments should be updated as what I suggested above...

> +   "char *name" for thread name (need be allocated by caller).  */
> +
> +extern int linux_proc_get_thread_name (pid_t pid, pid_t tid,
> +                                       char *name, size_t size);
> +
>   #endif /* COMMON_LINUX_PROCFS_H */
> diff -ruN ./gdb.org/linux-nat.c ./gdb/linux-nat.c
> --- ./gdb.org/linux-nat.c       2013-02-13 22:59:49.000000000 +0800
> +++ ./gdb/linux-nat.c   2013-08-12 16:47:26.000000000 +0800
> @@ -4294,6 +4294,14 @@
>
>         fclose (comm_file);
>       }
> +    else
> +    {
> +      static char comm[COMM_LEN + 1];
> +      int size = linux_proc_get_thread_name (pid, lwp, comm, COMM_LEN+1);

A blank line is needed.  Space is needed before and after "+".

The type of pid is int and type of lwp is long, but the types of 
parameters of linux_proc_get_thread_name are pid_t and pid_t.  It is 
inconsistent.

--
Yao (éå)