From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752711AbcHIPdL (ORCPT ); Tue, 9 Aug 2016 11:33:11 -0400 Received: from h2.hallyn.com ([78.46.35.8]:40092 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752238AbcHIPdH (ORCPT ); Tue, 9 Aug 2016 11:33:07 -0400 Date: Tue, 9 Aug 2016 10:33:05 -0500 From: "Serge E. Hallyn" To: Tejun Heo Cc: gregkh@linuxfoundation.org, serge.hallyn@ubuntu.com, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, kernel-team@fb.com, hannes@cmpxchg.org, lizefan@huawei.com Subject: Re: [PATCH 2/4] kernfs: make kernfs_path*() behave in the style of strlcpy() Message-ID: <20160809153305.GB30775@mail.hallyn.com> References: <1470720204-4605-1-git-send-email-tj@kernel.org> <1470720204-4605-3-git-send-email-tj@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470720204-4605-3-git-send-email-tj@kernel.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Tejun Heo (tj@kernel.org): > kernfs_path*() functions always return the length of the full path but > the path content is undefined if the length is larger than the > provided buffer. This makes its behavior different from strlcpy() and > requires error handling in all its users even when they don't care > about truncation. In addition, the implementation can actully be > simplified by making it behave properly in strlcpy() style. > > * Update kernfs_path_from_node_locked() to always fill up the buffer > with path. If the buffer is not large enough, the output is > truncated and terminated. > > * kernfs_path() no longer needs error handling. Make it a simple > inline wrapper around kernfs_path_from_node(). > > * sysfs_warn_dup()'s use of kernfs_path() doesn't need error handling. > Updated accordingly. > > * cgroup_path()'s use of kernfs_path() updated to retain the old > behavior. > > Signed-off-by: Tejun Heo > Cc: Serge Hallyn > Cc: Greg Kroah-Hartman > --- > fs/kernfs/dir.c | 61 ++++++++++++++------------------------------------ > fs/sysfs/dir.c | 6 ++--- > include/linux/cgroup.h | 7 +++++- > include/linux/kernfs.h | 21 ++++++++++++----- > 4 files changed, 42 insertions(+), 53 deletions(-) > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c > index e57174d..09242aa 100644 > --- a/fs/kernfs/dir.c > +++ b/fs/kernfs/dir.c > @@ -110,8 +110,9 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a, > * kn_to: /n1/n2/n3 [depth=3] > * result: /../.. > * > - * return value: length of the string. If greater than buflen, > - * then contents of buf are undefined. On error, -1 is returned. > + * Returns the length of the full path. If the full length is equal to or > + * greater than @buflen, @buf contains the truncated path with the trailing > + * '\0'. On error, -errno is returned. > */ > static int kernfs_path_from_node_locked(struct kernfs_node *kn_to, > struct kernfs_node *kn_from, > @@ -119,9 +120,8 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to, > { > struct kernfs_node *kn, *common; > const char parent_str[] = "/.."; > - size_t depth_from, depth_to, len = 0, nlen = 0; > - char *p; > - int i; > + size_t depth_from, depth_to, len = 0; > + int i, j; > > if (!kn_from) > kn_from = kernfs_root(kn_to)->kn; > @@ -131,7 +131,7 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to, > > common = kernfs_common_ancestor(kn_from, kn_to); > if (WARN_ON(!common)) > - return -1; > + return -EINVAL; > > depth_to = kernfs_depth(common, kn_to); > depth_from = kernfs_depth(common, kn_from); > @@ -144,22 +144,16 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to, > len < buflen ? buflen - len : 0); > > /* Calculate how many bytes we need for the rest */ > - for (kn = kn_to; kn != common; kn = kn->parent) > - nlen += strlen(kn->name) + 1; > - > - if (len + nlen >= buflen) > - return len + nlen; > - > - p = buf + len + nlen; > - *p = '\0'; > - for (kn = kn_to; kn != common; kn = kn->parent) { > - size_t tmp = strlen(kn->name); > - p -= tmp; > - memcpy(p, kn->name, tmp); > - *(--p) = '/'; > + for (i = depth_to - 1; i >= 0; i--) { > + for (kn = kn_to, j = 0; j < i; j++) > + kn = kn->parent; This is O(n^2) where n is the path depth. It's not a hot path, though, do we care?