Re: [PATCH 1/8] kernfs: Add API to generate relative kernfs path

[Tejun Heo <tj@kernel.org>]

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