lkml.org 
[lkml]   [2015]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/8] kernfs: Add API to generate relative kernfs path
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


\
 
 \ /
  Last update: 2015-11-24 17:41    [W:1.404 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site