From mboxrd@z Thu Jan  1 00:00:00 1970
From: Cong Wang <xiyou.wangcong@gmail.com>
Subject: Re: [PATCH 2/7] mark root hnode explicitly
Date: Thu, 6 Sep 2018 20:23:36 -0700
Message-ID: <CAM_iQpV7H8v3Oebftd0_aHzwwU_Phr3-7a=Un2-TRmuuZr7VOw@mail.gmail.com>
References: <20180905190134.GQ19965@ZenIV.linux.org.uk> <20180905190414.5477-1-viro@ZenIV.linux.org.uk>
 <20180905190414.5477-2-viro@ZenIV.linux.org.uk> <8d3e140a-eb1b-3aae-d9e7-36d103a54e5e@mojatatu.com>
 <5689c19a-c4fb-24c1-4594-86f2639faa98@mojatatu.com> <20180906105933.GU19965@ZenIV.linux.org.uk>
 <CAM_iQpUDpKB2y0Yy5YJjj7MALGsDLJKpdXYZkydmCo1N_uo=FA@mail.gmail.com> <20180907030438.GX19965@ZenIV.linux.org.uk>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        Jiri Pirko <jiri@resnulli.us>
To: Al Viro <viro@zeniv.linux.org.uk>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pg1-f196.google.com ([209.85.215.196]:34691 "EHLO
        mail-pg1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1725948AbeIGICb (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 7 Sep 2018 04:02:31 -0400
Received: by mail-pg1-f196.google.com with SMTP id d19-v6so6266510pgv.1
        for <netdev@vger.kernel.org>; Thu, 06 Sep 2018 20:23:49 -0700 (PDT)
In-Reply-To: <20180907030438.GX19965@ZenIV.linux.org.uk>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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?