On Tue, Dec 08, 2015 at 10:52:51AM -0500, Tejun Heo wrote:
> 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?
Yup,
> WARN_ONCE()'s first argument is condition, so
> you'd write
>
> if (WARN_ONCE(!common, "blah blah"))
> return NULL;
Well that's odd, i thought I had changed all those to
pr_*. Will fix.
> > +
> > + 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?
Ok, will do.
--
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
