Re: [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets

[Joel Brobecker <brobecker@adacore.com>]

> Rather than relying on the default remotetimeout value (which might be
> too small for some slower devices), use the existing gdb_load_timeout
> config option to set it.

No one seems to be comfortable looking at this, so I took a look.
I think the idea makes sense.

> +	    if [is_remote target] {
> +		set oldremotetimeout [get_remotetimeout]
> +		set_remotetimeout $loadtimeout
> +	    }

Is there a specific reason you already know about that made you
change the remotetimeout only when the target is remote? If there is,
I think it'd be nice to add a comment to that effect. If it's just
because we don't expect the timeout to have any effect in the non-
remote case, then I guess we can leave it like that. But I'm not
to certain about that. I would consider the idea of making things
simpler by just setting the timeout everytime, since this part of
the code will always result in a "load" command being used.

> +proc get_remotetimeout { } {

This function needs a comment describing what it does. Same for
set_remotetimeout.

> +    send_gdb "show remotetimeout\n"
> +    gdb_expect {

Can you use gdb_test_multiple instead?

> +    # Pick the default that gdb uses
> +    return 300

I think we should at least get a warning when that happens, because
it means that the regexp in the check before failed to match.
If we don't have some kind of visual cue, I think it'll be easy
to not notice that something changed, and the function is now
returning a default value, rather than the real timeout at that
time...

> +    send_gdb "set remotetimeout $timeout\n";
> +    gdb_expect {

Same, can we use gdb_test_multiple?

-- 
Joel