Re: [PATCH 2/7] mark root hnode explicitly

On Thu, Sep 6, 2018 at 8:04 PM Al Viro [off-list ref] wrote:

On Thu, Sep 06, 2018 at 07:57:25PM -0700, Cong Wang wrote:

quoted

quoted

-       if (root_ht == ht) {
+       if (ht->is_root) {

What's wrong with comparing pointers with root ht?

The fact that there may be more than one tcf_proto sharing tp->data.

Hmm? root ht is from tp->root, not tp->data.

Also, this very important information is missing in your one-line changelog...

quoted

quoted

                NL_SET_ERR_MSG_MOD(extack, "Not allowed to delete root node");
                return -EINVAL;
        }

@@ -795,6 +797,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
                                NL_SET_ERR_MSG_MOD(extack, "Link hash table not found");
                                return -EINVAL;
                        }
+                       if (ht_down->is_root) {

root ht is saved in tp->root, so you can compare ht_down with it too,
if you want.

If this check is all what you need, you don't need an extra flag.

Again, *which* tp?  We can trivially check that we are not linking to/deleting

Pretty sure there is a 'tp' in u32_set_parms() parameter list.

Are you saying it is not what you want? If so, why?

More importantly, why this information is again missing in your
changelog? This patch is definitely not trivial, it deserves a detailed
changelog.

our own root, sure.  But there's nothing to stop doing the same via another
tcf_proto...

To my best knowledge, the place where you set ->is_root=true
is precisely same with where we set tp->root=root_ht, and it doesn't
change after set. What am I missing here?