From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45419 invoked by alias); 12 Feb 2019 02:43:14 -0000 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 Received: (qmail 45374 invoked by uid 89); 12 Feb 2019 02:43:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Feb 2019 02:43:08 +0000 Received: from [10.0.0.178] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id CD2FB1E077; Mon, 11 Feb 2019 21:43:05 -0500 (EST) Subject: Re: [PATCH v2 2/4] Add a new function child_path. To: John Baldwin , gdb-patches@sourceware.org References: From: Simon Marchi Message-ID: Date: Tue, 12 Feb 2019 02:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-02/txt/msg00128.txt.bz2 On 2019-01-28 3:47 p.m., John Baldwin wrote: > child_path returns a pointer to the first component in a child path > that comes after a parent path. This does not depend on trying to > stat() the paths since they may describe remote paths but instead > relies on filename parsing. The function requires that the child path > describe a filename that contains at least one component below the > parent path and returns a pointer to the first component. > > gdb/ChangeLog: > > * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add > unittests/child-path-selftests.c. > * common/pathstuff.c (child_path): New function. > * common/pathstuff.h (child_path): New prototype. > * unittests/child-path-selftests.c: New file. Thanks, this LGTM. Just minor comments below. > --- > gdb/ChangeLog | 8 ++++ > gdb/Makefile.in | 1 + > gdb/common/pathstuff.c | 52 ++++++++++++++++++++++ > gdb/common/pathstuff.h | 6 +++ > gdb/unittests/child-path-selftests.c | 64 ++++++++++++++++++++++++++++ > 5 files changed, 131 insertions(+) > create mode 100644 gdb/unittests/child-path-selftests.c > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 38d740e440..93a2cebe1c 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,11 @@ > +2019-01-28 John Baldwin > + > + * Makefile.in (SUBDIR_UNITTESTS_SRCS): Add > + unittests/child-path-selftests.c. > + * common/pathstuff.c (child_path): New function. > + * common/pathstuff.h (child_path): New prototype. > + * unittests/child-path-selftests.c: New file. > + > 2019-01-28 John Baldwin > > * symfile.c (find_separate_debug_file): Look for separate debug > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 72ca855eb0..cec7ad32a4 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -411,6 +411,7 @@ SUBDIR_PYTHON_CFLAGS = > > SUBDIR_UNITTESTS_SRCS = \ > unittests/array-view-selftests.c \ > + unittests/child-path-selftests.c \ > unittests/cli-utils-selftests.c \ > unittests/common-utils-selftests.c \ > unittests/copy_bitwise-selftests.c \ > diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c > index 11675303b3..d6beb8faf1 100644 > --- a/gdb/common/pathstuff.c > +++ b/gdb/common/pathstuff.c > @@ -147,6 +147,58 @@ gdb_abspath (const char *path) > > /* See common/pathstuff.h. */ > > +const char * > +child_path (const char *parent, const char *child) > +{ > + /* The child path must start with the parent path. */ > + size_t parent_len = strlen (parent); > + if (filename_ncmp (parent, child, parent_len) != 0) > + return NULL; > + > + /* The parent path must be a directory and the child must contain at > + least one component underneath the parent. */ > + const char *child_component; > + if (IS_DIR_SEPARATOR (parent[parent_len - 1])) > + { > + /* The parent path ends in a directory separator, so it is a > + directory. The first child component starts after the common > + prefix. */ > + child_component = child + parent_len; > + } > + else > + { > + /* The parent path does not end in a directory separator. The > + first character in the child after the common prefix must be > + a directory separator. > + > + Note that CHILD must hold at least parent_len characters for > + filename_ncmp to return zero. If the character at parent_len > + is nul due to CHILD containing the same path as PARENT, the > + IS_DIR_SEPARATOR check will fail here. */ > + if (!IS_DIR_SEPARATOR (child[parent_len])) > + return NULL; > + > + /* The first child component starts after the separator after the > + common prefix. */ > + child_component = child + parent_len + 1; > + } > + > + /* The child must contain at least one non-separator character after > + the parent. */ > + while (*child_component != '\0') > + { > + if (IS_DIR_SEPARATOR (*child_component)) > + { > + child_component++; > + continue; > + } > + return child_component; This might be simplified (avoid the continue) by reversing the logic, using this as the loop body: { if (!IS_DIR_SEPARATOR (*child_component)) return child_component; child_component++; } > + } > + return NULL; > +} > + > +/* See common/pathstuff.h. */ > + > bool > contains_dir_separator (const char *path) > { > diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h > index d43f337550..20a7bdda26 100644 > --- a/gdb/common/pathstuff.h > +++ b/gdb/common/pathstuff.h > @@ -48,6 +48,12 @@ extern gdb::unique_xmalloc_ptr > > extern gdb::unique_xmalloc_ptr gdb_abspath (const char *path); > > +/* If the path in CHILD is a child of the path in PARENT, return a > + pointer to the first component in the CHILD's pathname below the > + PARENT. Otherwise, return NULL. */ > + > +extern const char *child_path (const char *parent, const char *child); > + > /* Return whether PATH contains a directory separator character. */ > > extern bool contains_dir_separator (const char *path); > diff --git a/gdb/unittests/child-path-selftests.c b/gdb/unittests/child-path-selftests.c > new file mode 100644 > index 0000000000..2621eecef6 > --- /dev/null > +++ b/gdb/unittests/child-path-selftests.c > @@ -0,0 +1,64 @@ > +/* Self tests for child_path for GDB, the GNU debugger. > + > + Copyright (C) 2019 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include "defs.h" > +#include "common/pathstuff.h" > +#include "common/selftest.h" > + > +namespace selftests { > +namespace child_path { > + > +/* Verify the result of a single child_path test. */ > + > +static bool > +child_path_check (const char *parent, const char *child, const char *expected) > +{ > + const char *result = ::child_path (parent, child); > + if (result == NULL || expected == NULL) > + return result == expected; > + return strcmp (result, expected) == 0; > +} > + > +/* Test child_path. */ > + > +static void > +test () > +{ > + SELF_CHECK (child_path_check ("/one", "/two", NULL)); > + SELF_CHECK (child_path_check ("/one", "/one", NULL)); > + SELF_CHECK (child_path_check ("/one", "/one/", NULL)); > + SELF_CHECK (child_path_check ("/one", "/one//", NULL)); > + SELF_CHECK (child_path_check ("/one", "/one/two", "two")); > + SELF_CHECK (child_path_check ("/one/", "/two", NULL)); > + SELF_CHECK (child_path_check ("/one/", "/one", NULL)); > + SELF_CHECK (child_path_check ("/one/", "/one/", NULL)); > + SELF_CHECK (child_path_check ("/one/", "/one//", NULL)); > + SELF_CHECK (child_path_check ("/one/", "/one/two", "two")); > +} Here are a few more edge cases I could think of: SELF_CHECK (child_path_check ("/one/", "/one//two", "two")); SELF_CHECK (child_path_check ("/one/", "/one//two/", "two/")); SELF_CHECK (child_path_check ("/one", "/onetwo", NULL)); SELF_CHECK (child_path_check ("/one", "/onetwo/three", NULL)); > + > +} > +} > + > +void > +_initialize_child_path_selftests () > +{ > + selftests::register_test ("child_path", > + selftests::child_path::test); > +} > + > Simon