From: Simon Marchi <simark@simark.ca>
To: John Baldwin <jhb@FreeBSD.org>, gdb-patches@sourceware.org
Subject: Re: [PATCH v2 4/5] Support 'info proc' for native FreeBSD processes.
Date: Fri, 05 Jan 2018 03:20:00 -0000 [thread overview]
Message-ID: <eb25fb7c-c0e9-ce06-f818-3755cc197393@simark.ca> (raw)
In-Reply-To: <20180104014923.11899-5-jhb@FreeBSD.org>
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
next prev parent reply other threads:[~2018-01-05 3:20 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-04 1:50 [PATCH v2 0/5] Support for 'info proc' on FreeBSD cores and native John Baldwin
2018-01-04 1:50 ` [PATCH v2 2/5] Don't return stale data from fbsd_pid_to_exec_file for kernel processes John Baldwin
2018-01-05 2:57 ` Simon Marchi
2018-01-05 19:43 ` John Baldwin
2018-01-04 1:50 ` [PATCH v2 3/5] Use gdb::unique_xmalloc_ptr<> instead of a deleter that invokes free() John Baldwin
2018-01-05 2:58 ` Simon Marchi
2018-01-04 1:50 ` [PATCH v2 1/5] Support 'info proc' for FreeBSD process core dumps John Baldwin
2018-01-05 2:54 ` Simon Marchi
2018-01-05 19:43 ` John Baldwin
2018-01-09 19:29 ` Simon Marchi
2018-01-04 1:59 ` [PATCH v2 5/5] Document support for 'info proc' on FreeBSD John Baldwin
2018-01-04 16:38 ` Eli Zaretskii
2018-01-04 21:36 ` John Baldwin
2018-01-05 6:53 ` Eli Zaretskii
2018-01-04 1:59 ` [PATCH v2 4/5] Support 'info proc' for native FreeBSD processes John Baldwin
2018-01-05 3:20 ` Simon Marchi [this message]
2018-01-05 19:43 ` John Baldwin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eb25fb7c-c0e9-ce06-f818-3755cc197393@simark.ca \
--to=simark@simark.ca \
--cc=gdb-patches@sourceware.org \
--cc=jhb@FreeBSD.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).