Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753653AbbKXQQg (ORCPT ); Tue, 24 Nov 2015 11:16:36 -0500 Received: from mail-yk0-f172.google.com ([209.85.160.172]:36732 "EHLO mail-yk0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753255AbbKXQQe (ORCPT ); Tue, 24 Nov 2015 11:16:34 -0500 Date: Tue, 24 Nov 2015 11:16:30 -0500 From: Tejun Heo To: serge@hallyn.com Cc: linux-kernel@vger.kernel.org, adityakali@google.com, linux-api@vger.kernel.org, containers@lists.linux-foundation.org, cgroups@vger.kernel.org, lxc-devel@lists.linuxcontainers.org, akpm@linux-foundation.org, ebiederm@xmission.com Subject: Re: [PATCH 1/8] kernfs: Add API to generate relative kernfs path Message-ID: <20151124161630.GL17033@mtj.duckdns.org> References: <1447703505-29672-1-git-send-email-serge@hallyn.com> <1447703505-29672-2-git-send-email-serge@hallyn.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1447703505-29672-2-git-send-email-serge@hallyn.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3265 Lines: 122 Hello, On Mon, Nov 16, 2015 at 01:51:38PM -0600, serge@hallyn.com wrote: > +static char * __must_check kernfs_path_from_node_locked( > + struct kernfs_node *kn_from, > + struct kernfs_node *kn_to, > + char *buf, > + size_t buflen) > +{ > + char *p = buf; > + struct kernfs_node *kn; > + size_t depth_from = 0, depth_to, d; > int len; > > + /* We atleast need 2 bytes to write "/\0". */ > + BUG_ON(buflen < 2); I don't think this is BUG worthy. Just return NULL? Also, the only reason the original function returned char * was because the starting point may not be the start of the buffer which helps keeping the implementation simple. If this function is gonna be complex anyway, a better approach would be returning ssize_t and implement a simliar behavior to strlcpy(). > + /* Short-circuit the easy case - kn_to is the root node. */ > + if ((kn_from == kn_to) || (!kn_from && !kn_to->parent)) { > + *p = '/'; > + *(p + 1) = '\0'; Hmm... so if kn_from == kn_to, the output is "/"? > + return p; > + } > + > + /* We can find the relative path only if both the nodes belong to the > + * same kernfs root. > + */ > + if (kn_from) { > + BUG_ON(kernfs_root(kn_from) != kernfs_root(kn_to)); Ditto, just return NULL and maybe trigger WARN_ON_ONCE(). > + depth_from = kernfs_node_depth(kn_from); > + } > + > + depth_to = kernfs_node_depth(kn_to); > + > + /* We compose path from left to right. So first write out all possible ^ , so > + * "/.." strings needed to reach from 'kn_from' to the common ancestor. > + */ Please fully-wing multiline comments. > + if (kn_from) { > + while (depth_from > depth_to) { > + len = strlen("/.."); Maybe do something like the following instead? const char parent_str[] = "/.."; size_t len = sizeof(parent_str) - 1; > + if ((buflen - (p - buf)) < len + 1) { > + /* buffer not big enough. */ > + buf[0] = '\0'; > + return NULL; > + } > + memcpy(p, "/..", len); > + p += len; > + *p = '\0'; > + --depth_from; > + kn_from = kn_from->parent; > } > + > + d = depth_to; > + kn = kn_to; > + while (depth_from < d) { > + kn = kn->parent; > + d--; > + } > + > + /* Now we have 'depth_from == depth_to' at this point. Add more Ditto with winging. > + * "/.."s until we reach common ancestor. In the worst case, > + * root node will be the common ancestor. > + */ > + while (depth_from > 0) { > + /* If we reached common ancestor, stop. */ > + if (kn_from == kn) > + break; > + len = strlen("/.."); > + if ((buflen - (p - buf)) < len + 1) { > + /* buffer not big enough. */ > + buf[0] = '\0'; > + return NULL; > + } > + memcpy(p, "/..", len); > + p += len; > + *p = '\0'; > + --depth_from; > + kn_from = kn_from->parent; > + kn = kn->parent; > + } Hmmm... I wonder whether this and the above block can be merged. Wouldn't it be simpler to calculate common ancestor and generate /.. till it reached that point? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/