Re: [RFA] fix "server" command prefix handling (unexpected confirmation queries)

[Pedro Alves <palves@redhat.com>]

On 12/12/2017 07:05 AM, Joel Brobecker wrote:
> Hello,
> 
> The "server" command prefix no longer turns confirmation queries off.
> We can reproduce this with any program by tring to delete all breakpoints,
> for instance:
> 
>     (gdb) break main
>     Breakpoint 1 at 0x40049b: file /[...]/break-fun-addr1.c, line 21.
>     (gdb) server delete breakpoints
>     Delete all breakpoints? (y or n)
> 
> GDB should not be asking "Delete all breakpoints? (y or n)", but
> instead just delete all breakpoints without asking for confirmation.
> 
> Looking at utils.c::defaulted_query gives a glimpse of how this feature
> is expected to work:
> 
>   /* Automatically answer the default value if the user did not want
>      prompts or the command was issued with the server prefix.  */
>   if (!confirm || server_command)
>     return def_value;
> 
> So, it relies on the server_command global to be set when the "server "
> command prefix is used, which is no longer the case since the following
> commit:
> 
>     commit b69d38afdea34e4fecab5ea47ffe1e594e0b6233
>     Date:   Wed Mar 9 18:25:00 2016 +0000
>     Subject: Command line input handling TLC
> 
> The patch was simplifying the handling for the command line, and
> I believe there was just a small oversight of removing the setting
> of the server_command global.

Whoops, indeed.  Bleh, this goes to show that anything
not covered by tests is bound to get broken at some point, no
matter how trivial.  :-P  Thanks for adding the new testcase!

> 
> This patch restores that, and adds a testcase to make sure we test
> that feature.
> 
> gdb/ChangeLog:
> 
>         * event-top.c (handle_line_of_input): Set server_command.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.base/server-del-break.c: New file.
>         * gdb.base/server-del-break.exp: New file.
> 
> Tested on x86_64-linux, no regression.
> OK to push to master?

LGTM, with a couple easy tweaks to the testcase.  See below.

> +}
> diff --git a/gdb/testsuite/gdb.base/server-del-break.exp b/gdb/testsuite/gdb.base/server-del-break.exp
> new file mode 100644
> index 0000000..8005419
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/server-del-break.exp
> @@ -0,0 +1,34 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +

Please add a small intro comment here mentioning what
the testcase is exercising.

> +set testfile "server-del-break"
> +set srcfile ${testfile}.c
> +set binfile [standard_output_file ${testfile}]
> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested "failed to compile first testcase"
> +    return -1
> +}
> +
> +clean_restart ${binfile}

Replace all the above with standard_testfile + prepare_for_testing?

> +
> +gdb_test "break main" \
> +         "Breakpoint.*at.* file .*$srcfile, line .*"
> +
> +gdb_test_no_output "server delete breakpoints"

Maybe add a comment here, about making sure that "server"
suppresses the "Delete all breakpoints?" query?  It's not
immediately obvious that that's what this is testing if you
look at the testcase in isolation as is.  (Might be adding
an intro comment makes this one redundant though.)

Thanks,
Pedro Alves