Re: [PATCH/RFC 02/02 v2] Refactor PRPSINFO handling on GDB

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Sun, 30 Dec 2012 02:53:18 +0100, Sergio Durigan Junior wrote:
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index d5ad6e3..18b817f 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
[...]
> @@ -1153,6 +1155,197 @@ linux_corefile_thread_callback (struct thread_info *info, void *data)
>    return !args->note_data;
>  }
>  
> +/* Fill the PRPSINFO structure with information about the process being
> +   debugged.  Returns 1 in case of success, 0 for failures.  Please note that
> +   even if the structure cannot be entirely filled (e.g., GDB was unable to
> +   gather information about the process UID/GID), this function will still
> +   return 1 since some information was already recorded.  It will only return
> +   0 iff nothing can be gathered.  */
> +
> +static int
> +linux_fill_prpsinfo (struct elf_internal_prpsinfo *p)
> +{
> +  /* The filename which we will use to obtain some info about the process.
> +     We will basically use this to store the `/proc/PID/FILENAME' file.  */
> +  char filename[100];
> +  /* The full name of the program which generated the corefile.  */
> +  char *fname;
> +  /* The basename of the executable.  */
> +  const char *basename;
> +  /* The arguments of the program.  */
> +  char *psargs;
> +  char *infargs;
> +  /* The contents of `/proc/PID/stat' file.  */
> +  char *proc_stat, *proc_stat_orig;
> +  /* The valid states of a process, according to the Linux kernel.  */
> +  const char valid_states[] = "RSDTZW";
> +  /* The program state.  */
> +  char *prog_state;

It could be (also for C++ compliance with its overloaded strchr):
	const char *prog_state;


> +  /* The state of the process.  */
> +  char pr_sname;
> +  /* The PID of the program which generated the corefile.  */
> +  pid_t pid;
> +  /* Process flags.  */
> +  unsigned int pr_flag;
> +  /* Process nice value.  */
> +  long pr_nice;
> +  /* The stat of the `/proc/PID/stat' file.  */
> +  struct stat proc_st;
> +  /* The number of fields read by `sscanf'.  */
> +  int n_fields = 0;
> +  /* Cleanups.  */
> +  struct cleanup *c;
> +  int i;
> +  volatile struct gdb_exception ex;
> +
> +  gdb_assert (p != NULL);
> +
> +  /* Obtaining PID and filename.  */
> +  pid = ptid_get_pid (inferior_ptid);
> +  xsnprintf (filename, sizeof (filename), "/proc/%u/cmdline", pid);
> +  fname = target_fileio_read_stralloc (filename);
> +
> +  if (fname == NULL || *fname == '\0')
> +    {
> +      /* No program name was read, so we won't be able to retrieve more
> +	 information about the process.  */
> +      xfree (fname);
> +      return 0;
> +    }
> +
> +  c = make_cleanup (xfree, fname);
> +  memset (p, 0, sizeof (*p));
> +
> +  /* Obtaining the file stat as well.  */
> +  errno = 0;
> +  if (stat (filename, &proc_st) != 0)

A nitpick but a failed syscall has to initialize ERRNO so the explicit
initialization is not needed.

(It is used for example for ptrace(PTRACE_PEEKTEXT) where one cannot find out
whether the syscall was successful or not from the ptrace return value.)


> +    {
> +      warning (_("Could not stat file `%s': %s"), filename,
> +	       safe_strerror (errno));
> +      p->pr_uid = 0;
> +      p->pr_gid = 0;
> +    }
> +  else
> +    {
> +      p->pr_uid = proc_st.st_uid;
> +      p->pr_gid = proc_st.st_gid;
> +    }
> +
> +  /* Defining the PID.  */
> +  p->pr_pid = pid;
> +
> +  /* Copying the program name.  Only the basename matters.  */
> +  basename = lbasename (fname);
> +  strncpy (p->pr_fname, basename, sizeof (p->pr_fname));
> +  p->pr_fname[sizeof (p->pr_fname) - 1] = '\0';
> +
> +  /* Generating and copying the program's arguments.  `get_inferior_args'
> +     may throw, but we want to continue the execution anyway.  */
> +  TRY_CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      infargs = get_inferior_args ();
> +    }
> +
> +  if (ex.reason < 0)
> +    {
> +      warning (_("Could not obtain inferior's arguments."));

You could print also ex.message here.


> +      infargs = NULL;
> +    }
> +
> +  psargs = xstrdup (fname);
> +  if (infargs != NULL)
> +    psargs = reconcat (psargs, psargs, " ", infargs, NULL);
> +
> +  make_cleanup (xfree, psargs);
> +
> +  strncpy (p->pr_psargs, psargs, sizeof (p->pr_psargs));
> +  p->pr_psargs[sizeof (p->pr_psargs) - 1] = '\0';
> +
> +  xsnprintf (filename, sizeof (filename), "/proc/%u/stat", pid);

Originally:
# > +  xsnprintf (filename, sizeof (filename), "/proc/%d/stat", pid);
# 
# pid is pid_t, I believe on some systems it may be incompatible with %d.

Sorry I did mean to use:
	xsnprintf (filename, sizeof (filename), "/proc/%d/stat", (int) pid);


> +  proc_stat = target_fileio_read_stralloc (filename);

Moving make_cleanup (xfree, proc_stat); here will not need xfree (proc_stat);
below.


> +
> +  if (proc_stat == NULL || *proc_stat == '\0')
> +    {
> +      /* Despite being unable to read more information about the process, we
> +	 return 1 here because at least we have its command line, PID and
> +	 arguments.  */
> +      xfree (proc_stat);
> +      do_cleanups (c);
> +      return 1;
> +    }
> +
> +  proc_stat_orig = proc_stat;
> +  make_cleanup (xfree, proc_stat_orig);

Probably some leftover, it is enough to call:
	make_cleanup (xfree, proc_stat);

PROC_STAT is passed by value, not by reference.


> +
> +  /* Ok, we have the stats.  It's time to do a little parsing of the contents
> +     of the buffer, so that we end up reading what we want.
> +
> +     The following parsing mechanism is strongly based on the information
> +     generated by the `fs/proc/array.c' file, present in the Linux kernel
> +     tree.  More details about how the information is displayed can be
> +     obtained by seeing the manpage of proc(5), specifically under the entry
> +     of `/proc/[pid]/stat'.  */
[...]


Thanks,
Jan