Re: [PATCH v2 4/5] Support 'info proc' for native FreeBSD processes.

[Simon Marchi <simark at simark dot ca>]

On 2018-01-03 08:49 PM, John Baldwin wrote:
> - Command line arguments are fetched via the kern.proc.args.<pid>
>   sysctl.
> - The 'cwd' and 'exe' values are obtained from the per-process
>   file descriptor table returned by kinfo_getfile() from libutil.
> - 'mappings' is implemented by walking the array of VM map entries
>   returned by kinfo_getvmmap() from libutil.
> - 'status' output is generated by outputting fields from the structure
>   returned by the kern.proc.pid.<pid> sysctl.
> - 'stat' is aliased to 'status'.

Hi John,

The patch LGTM in general, I noted 2 comments, the first one could be done
as a separate patch (after this series), if you agree with it.  The second,
I'm fine if you just fix it before pushing.

> +	  struct kinfo_vmentry *kve = vmentl.get ();
> +	  for (int i = 0; i < nvment; i++, kve++)
> +	    {
> +	      ULONGEST start, end;
> +
> +	      start = kve->kve_start;
> +	      end = kve->kve_end;
> +#ifdef __LP64__
> +	      printf_filtered ("  %18s %18s %10s %10s %9s %s\n",
> +			       hex_string (start),
> +			       hex_string (end),
> +			       hex_string (end - start),
> +			       hex_string (kve->kve_offset),
> +			       fbsd_vm_map_entry_flags (kve->kve_flags,
> +							kve->kve_protection),
> +			       kve->kve_path);
> +#else
> +	      printf_filtered ("\t%10s %10s %10s %10s %9s %s\n",
> +			       hex_string (start),
> +			       hex_string (end),
> +			       hex_string (end - start),
> +			       hex_string (kve->kve_offset),
> +			       fbsd_vm_map_entry_flags (kve->kve_flags,
> +							kve->kve_protection),
> +			       kve->kve_path);
> +#endif
> +	    }

It might be a good idea to factor out the printing of vm map entries from here
and the core code, to make sure that they will always be printed the same way.
It's up to you.

> +	}
> +      else
> +	warning (_("unable to fetch virtual memory map"));
> +    }
> +#endif
> +  if (do_status)
> +    {
> +      if (!fbsd_fetch_kinfo_proc(pid, &kp))

Missing space here.  But didn't we fetch it already (line 329)?
IIUC, we only need it in this scope, so you could move the
relevant variables here, and only call fbsd_fetch_kinfo_proc here.
I suppose it needed to be this way when stat and status were not
merged.

> +	warning (_("Failed to fetch process information"));
> +      else

Thanks,

Simon