From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15903 invoked by alias); 29 Jun 2011 16:25:54 -0000 Received: (qmail 15889 invoked by uid 22791); 29 Jun 2011 16:25:53 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 29 Jun 2011 16:25:30 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 2F7682BB36F; Wed, 29 Jun 2011 12:25:30 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id luzIOp-tI6gh; Wed, 29 Jun 2011 12:25:30 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id C5F792BB3DA; Wed, 29 Jun 2011 12:25:29 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 604A6145615; Wed, 29 Jun 2011 09:25:27 -0700 (PDT) Date: Wed, 29 Jun 2011 16:25:00 -0000 From: Joel Brobecker To: Mike Frysinger Cc: gdb-patches@sourceware.org, toolchain-devel@blackfin.uclinux.org, Jie Zhang Subject: Re: [PATCH v2] gdb: tests: set remotetimeout to gdb_load_timeout for remote targets Message-ID: <20110629162527.GC2407@adacore.com> References: <1307309678-10966-1-git-send-email-vapier@gentoo.org> <1308510312-5512-1-git-send-email-vapier@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1308510312-5512-1-git-send-email-vapier@gentoo.org> User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-06/txt/msg00455.txt.bz2 > 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