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

Joel Brobecker
2011-06-29T16:25:54Z

> 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