From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20938 invoked by alias); 1 Apr 2002 09:43:39 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 20918 invoked from network); 1 Apr 2002 09:43:35 -0000 Received: from unknown (HELO is.elta.co.il) (199.203.121.2) by sources.redhat.com with SMTP; 1 Apr 2002 09:43:35 -0000 Received: from is (is [199.203.121.2]) by is.elta.co.il (8.9.3/8.8.8) with SMTP id MAA23208; Mon, 1 Apr 2002 12:42:14 +0200 (IST) Date: Mon, 01 Apr 2002 01:43:00 -0000 From: Eli Zaretskii X-Sender: eliz@is To: Joel Brobecker cc: gdb-patches@sources.redhat.com Subject: Re: [RFC] xfullpath and new regression test xfullpath.exp In-Reply-To: <20020401105021.A13168@act-europe.fr> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2002-04/txt/msg00001.txt.bz2 On Mon, 1 Apr 2002, Joel Brobecker wrote: > I implemented the new xfullpath function based on Andrew's comments, and > made some adjustements to fix the problem reported in > > http://sources.redhat.com/ml/gdb-patches/2002-03/msg00345.html > > while being careful not to break Tom's setup. The patch is attached. Thanks. However, this patch will not DTRT on DOS and Windows systems, I'm afraid. Details below. > * source.c (openp): Use xfullpath in place of gdb_realpath to > avoid resolving the basename part of filenames when the > associated file is a symbolic link. This fixes a potential > inconsistency between the filenames known to GDB and the > filenames it prints in the annotations. This kind of detailed description should go into the source, IMHO. It's okay to have it in the ChangeLog as well, but having it _only_ in the ChangeLog is not a good idea: people who read the code don't expect to find the associated comments in the logs. + /* + * xfullpath + * + * Return a copy of FILENAME, with its directory prefix canonicalized + * by gdb_realpath. + */ I believe this is not the comment style the GNU project uses. Here are specific comments about the code part of your patch: + char * + xfullpath (const char *filename) + { + const char *base_name = lbasename (filename); + char *dir_name; + char *real_path; + char *result; + + /* Extract the basename of filename, and return immediately + a copy of filename if it does not contain any directory prefix. */ + if (base_name == filename) + return xstrdup (filename); + + dir_name = alloca ((size_t) (base_name - filename + 1)); + strncpy (dir_name, filename, base_name - filename); + dir_name[base_name - filename] = '\000'; This doesn't work correctly when the input name is something like "d:foo". lbasename returns a pointer to `f', so you end up with the directory being "d:", and the rest of the code will generate "d:/foo", which is something very different in semantics. The Right Thing to do in this case is to return either the unmodified "d:foo" or "d:./foo". + /* Canonicalize the directory prefix, and build the resulting + filename. Avoid returning a path which starts with 2 SLASH_CHARS + if the canonicalized path is the root path */ + real_path = gdb_realpath (dir_name); + if (strcmp (real_path, SLASH_STRING) == 0) + result = concat (SLASH_STRING, base_name, NULL); + else + result = concat (real_path, SLASH_STRING, base_name, NULL); This doesn't work if the input is "\\", which is also a root directory. You should use the more portable IS_DIR_SEPARATOR macro (defined on include/filenames.h) instead. Finally, I don't understand why do you avoid duplicate slashes when dir_name is "/", but not if it is "/foo/bar/". Why not handle the other case as well? (This issue is a general one, not something specific to MS-Windows, although the case of "d:/", the root directory on drive d:, was what initially brought that to my attention.)