From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21159 invoked by alias); 24 Oct 2007 18:20:51 -0000 Received: (qmail 21139 invoked by uid 22791); 24 Oct 2007 18:20:49 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 24 Oct 2007 18:20:46 +0000 Received: (qmail 30420 invoked from network); 24 Oct 2007 18:20:44 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 24 Oct 2007 18:20:44 -0000 To: Carlos O'Donell Cc: gdb-patches@sourceware.org, Daniel Jacobowitz Subject: Re: [PATCH] Fix uninitialized use of variables. References: <20071020172137.GC28823@lios> <20071024175743.GP21241@lios> From: Jim Blandy Date: Wed, 24 Oct 2007 18:21:00 -0000 In-Reply-To: <20071024175743.GP21241@lios> (Carlos O'Donell's message of "Wed, 24 Oct 2007 13:57:43 -0400") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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: 2007-10/txt/msg00597.txt.bz2 Carlos O'Donell writes: > On Tue, Oct 23, 2007 at 04:20:12PM -0700, Jim Blandy wrote: >> >> Carlos O'Donell writes: >> > Index: gdb/remote.c >> > =================================================================== >> > RCS file: /cvs/src/src/gdb/remote.c,v >> > retrieving revision 1.271 >> > diff -u -p -r1.271 remote.c >> > --- gdb/remote.c 8 Oct 2007 12:55:09 -0000 1.271 >> > +++ gdb/remote.c 18 Oct 2007 16:34:05 -0000 >> > @@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff, /* packet >> > static char * >> > unpack_nibble (char *buf, int *val) >> > { >> > - ishex (*buf++, val); >> > + if (!ishex (*buf++, val)) >> > + error (_("Unpacked nibble does not contain hex characters.")); >> > return buf; >> > } >> >> This looks fine to me, although Daniel has thoughts on error handling >> in the remote protocol that I don't fully understand. >> >> But the error message is going to be obscure to users. It should at >> least say something about the remote protocol packet being misformed. > > In all truth I would have rather returned a status and let the caller > determine the error message. I can see the appeal, but that sounds like a lot of work: - Change all the 'unpack_' functions to take the text pointer by reference, update it in place, and return a status. - Change all their callers to pass the text pointer by reference, check the error code, and return an error status. And the win would be that you can say specifically what part of the packet didn't parse, instead of just saying that the packet didn't parse. It would be cleaner overall, but speaking for myself, there are other things I'd appreciate more. :) The callers of the 'unpack_' functions currently seem to assume that the callees will raise errors as needed; it doesn't seem too bad to go along with that.