Re: [PATCH 1/4] Implement 'set honor-dontdump-flag' command

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, November 28 2017, Sergio Lopez wrote:

> Commit df8411da087dc05481926f4c4a82deabc5bc3859 implemented support for
> checking /proc/PID/coredump_filter, and also changed gcore behavior to
> unconditionally honor the VM_DONTDUMP flag, preventing sections marked
> as such for being dumped into the core file.
>
> This patch adds support for the 'set honor-dontdump-flag' command for
> instructing gdb to ignore the VM_DONTDUMP flag. Combined with 'set
> use-coredump-filter', this allows the user to restore the old behavior,
> dumping all sections (except the ones marked as IO) unconditionally.

Thanks for the patch.  A few comments below.

> ChangeLog

This should say "gdb/ChangeLog:".

> 2017-11-28  Sergio Lopez  <slp@redhat.com>
>
>         * linux-tdep.c (honor_dontdump_flag): New variable.
>         (dump_mapping_p): Use honor_dontdump_flag variable.
>         (_initialize_linux_tdep): New command 'set honor-dontdump-flag'.

Not sure if it was your MUA, but ChangeLog lines should be indented
using TABs.

> ---
>  gdb/ChangeLog    |  6 ++++++
>  gdb/linux-tdep.c | 19 ++++++++++++++++++-
>  2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ebb969998c..ce68fee776 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2017-11-28  Sergio Lopez  <slp@redhat.com>
> +
> +	* linux-tdep.c (honor_dontdump_flag): New variable.
> +	(dump_mapping_p): Use honor_dontdump_flag variable.
> +	(_initialize_linux_tdep): New command 'set honor-dontdump-flag'.

Here they're fine.  We don't usually include diffs to ChangeLogs in the
commit because they can cause conflicts when applying the patch, but
it's up to you.

> +
>  2017-11-27  Tom Tromey  <tom@tromey.com>
>  
>  	* Makefile.in (REMOTE_OBS): Remove.
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 24237b8d39..5f4a1cdad1 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -93,6 +93,11 @@ struct smaps_vmflags
>  
>  static int use_coredump_filter = 1;
>  
> +/* Whether to honor the VM_DONTDUMP flag in /proc/PID/smaps when
> +   generating a corefile.  */
> +
> +static int honor_dontdump_flag = 1;

No empty line between command and definition of variable.

> +
>  /* This enum represents the signals' numbers on a generic architecture
>     running the Linux kernel.  The definition of "generic" comes from
>     the file <include/uapi/asm-generic/signal.h>, from the Linux kernel
> @@ -655,7 +660,7 @@ dump_mapping_p (filter_flags filterflags, const struct smaps_vmflags *v,
>  	return 0;
>  
>        /* Check if we should exclude this mapping.  */
> -      if (v->exclude_coredump)
> +      if (honor_dontdump_flag && v->exclude_coredump)
>  	return 0;
>  
>        /* Update our notion of whether this mapping is shared or
> @@ -2517,4 +2522,16 @@ of /proc/PID/coredump_filter when generating the corefile.  For more information
>  about this file, refer to the manpage of core(5)."),
>  			   NULL, show_use_coredump_filter,
>  			   &setlist, &showlist);
> +
> +  add_setshow_boolean_cmd ("honor-dontdump-flag", class_files,
> +			   &honor_dontdump_flag, _("\
> +Set whether gcore should honor the VM_DONTDUMP flag."),
> +			   _("\
> +Show whether gcore should honor the VM_DONTDUMP flag."),
> +			   _("\
> +Use this command to set whether gcore should honor the VM_DONTDUMP\n\
> +flag from /proc/PID/smaps when generating the corefile.  For more information\n\
> +about this file, refer to the manpage of proc(5) and core(5)."),
> +			   NULL, show_use_coredump_filter,

You've already spotted the mistake of using 'show_use_coredump_filter'
here.

> +			   &setlist, &showlist);

I'm not sure "honor-dontdump-flag" is a good name for this setting.
There's no indication that it relates to coredumps, and I think it
should.  A name like "honor-coredump-dontdump-flag" is a bit repetitive,
but IMHO is better than just "honor-dontdump-flag".  WDYT?

To be fair, my first thought was to suggest adding a new "set coredump"
super command, which would encompass "set coredump use-coredump-filter"
and "set coredump honor-dontdump-flag", but I wouldn't like to introduce
this change so close to a release.

Otherwise, the patch looks OK to me, but I'm not a global maintainer.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/