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

[Cong Wang <xiyou.wangcong@gmail.com>]

On Thu, Sep 6, 2018 at 8:04 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Sep 06, 2018 at 07:57:25PM -0700, Cong Wang wrote:
>
> > > -       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...



>
> > >                 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?