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

[Tejun Heo <tj@kernel.org>]

Hello, Serge.

On Mon, Dec 07, 2015 at 05:06:16PM -0600, serge.hallyn@ubuntu.com wrote:
> +/* kernfs_node_depth - compute depth from @from to @to */
> +static size_t kernfs_node_distance(struct kernfs_node *from, struct kernfs_node *to)
>  {
> +	size_t depth = 0;
>  
> +	BUG_ON(!to);
> +	BUG_ON(!from);

Do these BUG_ON()s achieve anything?  Also, would something like
kernfs_relative_depth() be a better name for the function?  Maybe even
just kernfs_depth()?

...
> +static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
> +		struct kernfs_node *b)
> +{
> +	size_t da = kernfs_node_distance(kernfs_root(a)->kn, a);
> +	size_t db = kernfs_node_distance(kernfs_root(b)->kn, b);
> +
> +	if (da == 0)
> +		return a;
> +	if (db == 0)
> +		return b;

Hmm... are the above two ifs necessary?  Wouldn't the outcome be the
same?  Furthermore, if a and b are on different roots the above may
give the wrong answer while not doing the above would return NULL.

> +	while (da > db) {
> +		a = a->parent;
> +		da--;
> +	}
> +	while (db > da) {
> +		b = b->parent;
> +		db--;
> +	}
> +
> +	/* worst case b and a will be the same at root */
> +	while (b != a) {
> +		b = b->parent;
> +		a = a->parent;
> +	}
> +
> +	return a;
> +}
...
> +static char *
> +__must_check kernfs_path_from_node_locked(struct kernfs_node *kn_from,

Maybe

static char * __must_check
kernfs_path...

> +					  struct kernfs_node *kn_to, char *buf,
> +					  size_t buflen)

Given that @kn_from is optional and is not the target node, maybe put
@kn_to before @kn_from?

> +{
> +	char *p = buf;
> +	struct kernfs_node *kn, *common;
> +	const char parent_str[] = "/..";
> +	int i;
> +	size_t depth_from, depth_to, len = 0, nlen = 0,
> +	       plen = sizeof(parent_str) - 1;

Heh, idk, just put plen on a separate decl?

> +
> +	/* We atleast need 2 bytes to write "/\0". */
> +	if (buflen < 2)
> +		return NULL;
> +
> +	if (!kn_from)
> +		kn_from = kernfs_root(kn_to)->kn;
> +
> +	if (kn_from == kn_to) {
> +		*p = '/';
> +		*(++p) = '\0';
> +		return buf;
> +	}
> +
> +	common = kernfs_common_ancestor(kn_from, kn_to);
> +	if (!common) {
> +		WARN_ONCE("%s: kn_from and kn_to on different roots\n",
> +			__func__);
> +		return NULL;
> +	}

Have you compiled it?  WARN_ONCE()'s first argument is condition, so
you'd write

	if (WARN_ONCE(!common, "blah blah"))
		return NULL;

> +
> +	depth_to = kernfs_node_distance(common, kn_to);
> +	depth_from = kernfs_node_distance(common, kn_from);
> +
> +	for (i = 0; i < depth_from; i++) {
> +		if (len + plen + 1 > buflen)
> +			return NULL;
> +		strcpy(p, parent_str);
> +		p += plen;
> +		len += plen;
> +	}
> +
> +	/* 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 + 1 > buflen)
> +		return NULL;

Hmm... if we do this anyway, maybe we can make the function behave
more like other string formatting function (strlcpy) and return the
would-be length instead where ret >= len indicates truncation?

Thanks.

-- 
tejun