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

Pedro Alves
2017-11-28T16:21:48Z

On 11/28/2017 03:42 PM, Sergio Durigan Junior wrote:

>> @@ -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?

Personally, I don't find the "coredump" issue that much of a
problem, given "dump" is there. "honor-coredump-dontdump" looks
like a mouthful to me.

I think using "ignore" in the name would some more natural
in GDB than "honor". I.e., "set ignore-dontdump-flag on/off".
That mean default is off/0 ( and the control variable could go
to .bss :-) )

It also avoids our UK friends cursing at bad spelling of "honour". :-)

"set dump-excluded-mappings on/off" could work too?

I skimmed the series and didn't find a gdb/NEWS entry; we need
one for new commands.

>
> 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,
Pedro Alves