Hello, Serge.
On Mon, Dec 07, 2015 at 05:06:16PM -0600, serge.hallyn@xxxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
