Re: [PATCH] Handle correctly passing a bad interpreter name to new-ui

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-07-19 01:34 PM, Pedro Alves wrote:
> On 07/13/2016 03:13 PM, Simon Marchi wrote:
>> When a bad interpreter name is passed to new-ui, such as:
>>
>>   (gdb)  new-ui bloop /dev/pts/10
>>
>> A partially created UI is left in the UI list, with interp set to NULL.
>> Trying to do anything that will print on this UI (such as "start") will
>> cause a segmentation fault.
> 
> Whoops.  Thanks for fixing this.
> 
>>
>> gdb/ChangeLog:
>>
>> 	* top.h (make_delete_ui_cleanup): New declaration.
>> 	* top.c (delete_ui_cleanup): New function.
>> 	(make_delete_ui_cleanup): New function.
>> 	(new_ui_command): Create restore_ui cleanup earlier, create a
>> 	delete_ui cleanup and discard it on success.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.base/new-ui.exp (do_test_invalid_interpreter): New
>> 	procedure.
>> ---
>>  gdb/testsuite/gdb.base/new-ui.exp | 23 +++++++++++++++++++++++
>>  gdb/top.c                         | 34 +++++++++++++++++++++++++---------
>>  gdb/top.h                         |  3 +++
>>  3 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/new-ui.exp b/gdb/testsuite/gdb.base/new-ui.exp
>> index f3f66db..ddff8c4 100644
>> --- a/gdb/testsuite/gdb.base/new-ui.exp
>> +++ b/gdb/testsuite/gdb.base/new-ui.exp
>> @@ -143,4 +143,27 @@ proc do_test {} {
>>      }
>>  }
>>  
>> +proc do_test_invalid_interpreter {} {
> 
> An intro comment would be nice.  Something like the
> "wrong interpreter" comment below.

Right, that comment makes more sense here.

>> +    global testfile
>> +
>> +    clean_restart $testfile
>> +
>> +    spawn -pty
>> +    set extra_tty_name $spawn_out(slave,name)
>> +
>> +    if ![runto_main] {
>> +	untested "could not run to main"
>> +	return -1
>> +    }
> 
> Is this runto_main needed?

Woops no, copy-pasta.

>> +
>> +    # Test that asking for a wrong interpreter is properly reported.
>> +    gdb_test "new-ui bloop $extra_tty_name" "Interpreter `bloop' unrecognized"
> 
> lowercase "Interpreter".

Well, that's the actual output from GDB, if I change it it won't match anymore :).

> Did you consider instead adding this to do_test, just before
> the "new-ui console" test?  We do a couple other basic "new-ui"
> tests there too, like the no args, and no-repeat checks.  Seems this
> fits right in.  Since do_test also runs execution commands that print to
> both consoles, that would give the same coverage.  I ask because do_test is
> the driver procedure for the real tests, and with the new procedure we
> now have multiple clean_restart calls with no with_test_prefix, which is
> something that pedantically doesn't look 100% right to me because it can
> cause duplicate test messages (on internal errors, etc.).

I don't mind, it's just that I like to have small, independent test sequences in
each exp.  It's easier to analyze when you try to fix a test than when you have a
single big sequence, where you need to weed out what's relevant to what is failing
and what's not.  But you are right, we should use with_test_prefix at the root level.

Perhaps it would make sense to put all the "invalid usages" tests in the separate
procedure then, and leave the main procedure to test the real stuff.  We could also
factor out the extra pty creation, or just create a single one that we pass to all
the procedures.

I'll send a v2 based on your upcoming comments.

Thanks!

Simon