On 12/10/2015 05:35 PM, Richard Alpe wrote: > On 2015-12-09 16:37, Jon Maloy wrote: >> Ok. You evidently have spent more time than me pondering about this, so I >> have no more objections. > Okey, then I think I will go with address as identifier for a peer. > "tipc peer remove address ADDRESS" and eventually "tipc peer list" where > address > of course could be used to identify a single peer "tipc peer list address > ADDRESS" > just like "tipc link stat show link LINK" and "ip link list dev DEV" does > today. > > What I still don't know is what to call the kernel option. > TIPC_NL_NODE_REMOVE or TIPC_NL_PEER_REMOVE? > > I'm leaning towards peer remove. > Do you have any input on this Ying? >
Richard, you have made such lots of investigations and analysis like you have demonstrated below. My opinion is same as Jon, and I have no any objections about your proposals. I agree that "node" is a general concept as Jon said. But if we see "node" concept from a TIPC user's perspective, having it represent a remote endpoint seems more reasonable. So changing "node" as "peer" makes sense too. If so, I prefer TIPC_NL_PEER_REMOVE rather than TIPC_NL_NODE_REMOVE. Regards, Ying > Regards > Richard > >> >> ///jon >> >>> -----Original Message----- >>> From: Richard Alpe >>> Sent: Wednesday, 09 December, 2015 09:43 >>> To: Jon Maloy; [email protected] >>> Cc: Ying Xue >>> Subject: Re: [PATCH iproute2] peer (node) removal from userspace >>> >>> On 2015-12-09 13:48, Jon Maloy wrote: >>>> >>>>> -----Original Message----- >>>>> From: Richard Alpe >>>>> Sent: Wednesday, 09 December, 2015 05:07 >>>>> To: [email protected] >>>>> Cc: Jon Maloy; Ying Xue; Richard Alpe >>>>> Subject: [PATCH iproute2] peer (node) removal from userspace >>>>> >>>>> I first implemented node remove as "tipc node remove address >>>>> ADDRESS", then I noticed there is a "tipc node set address ADDRESS". >>>>> These should of course be each others counterparts. This made me >>>>> realise that "node remove" is really a "peer remove". So it became >>>>> "tipc peer remove address 1.1.2", >>>> >>>> To me "node" always meant node in general, i.e., both peer AND own >>> node. >>> Okey. >>> >>>> >>>>> where address is used as peer identifier. >>>> >>>> When an address it is used to identify the a node there is no need to >>>> specify the keyword "address"; it is not the address of the node you >>>> are removing, it is the node object itself. So why not just "tipc node >>> ADDRESS remove" ? >>> I initially wrote a long email explaining scalability. But I recall already >>> sending >>> you one of these. So I will try to put it shorter. >>> >>> Regarding implicit arguments >>> ---------------------------- >>> $ tipc node 1.2.3 remove >>> That removes a node. Super easy to understand and read, yes. But how does >>> it scale? >>> >>> Lets look at setting the address. >>> $ tipc node 1.2.3 set address - doesn't look so appealing, right? >>> >>> So we stick with: >>> $ tipc node set address 1.2.3 >>> >>> I don't know if you have every written a parser, but you would end up here: >>> if { $word3 == "set" } { >>> node_set_addr() >>> } else if { string_looks_like_an_address($word3) } { >>> do_more_logic_to_find_what_to_do_with_address() >>> } >>> >>> I really hope you see why this would be a horrible thing. >>> >>> The ip command skipped there key-value approach in one single place, when >>> specifying the device. They probably argued exactly like you did "what else >>> could a link be?" and then someone came up with link groups in addition to >>> single devices. >>> >>> Lets look at how that ended up. >>> >>> $ ip link add link data0 dev foobar type vlan id 102 $ ip link delete foobar >>> >>> Nice, now lets name the vlan "dev" >>> >>> $ ip link add link data0 dev dev type vlan id 103 $ ip link delete dev >>> Command >>> line is not complete. Try option "help" >>> >>> As another "I WANT TO SHOOT MYSELF IN THE HEAD" exerciser, you could >>> also try naming the vlan "help" or "name" :) >>> >>> Now, take a look at the ip parsing code. >>> http://git.kernel.org/cgit/linux/kernel/git/shemminger/iproute2.git/tree/ip/i >>> plink.c?id=20ed7b24df05eadf83168d1d0ce0052a31380928#n185 >>> >>> There is special handling and fixes for this all over the place. >>> Totally unnecessary, if they had kept the key-value approach everywhere so >>> much of this could have been avoided and the command would have been >>> consistent. >>> >>> The way forward >>> --------------- >>> I still think "tipc peer list" and "tipc peer remove address 1.1.2" makes a >>> lot of >>> sense. "tipc node remove address" could in the future remove the node >>> address, and "tipc node remove netid" could remove the netid. If we ever >>> implement kernel support for this. >>> >>>> >>>> It is possible that this should be changed elsewhere too, for consistency. >>>> >>> No. 1. We can't change it now. 2. It's stupid. >>> >>>> >>>> or "tipc node ADDRESS set foo 123" >>>> >>>> This is also why, as earlier mentioned, I would prefer the term >>>> "identity" to "address" in those cases. We are really talking about a >>>> node identity here, not an address (of which each node may have several, >>> IP, MAC...). >>> "tipc node set address 1.2.3" could have been "tipc node set identity >>> 1.2.3", I >>> agree. >>> >>> /Richard >>> >>>> >>>> ///jon >>>> >>>>> >>>>> This also made me realize that "tipc node list" should really be "tipc >>>>> peer >>> list". >>>>> I guess we should implement "tipc peer list" as a clone of "tipc node >>>>> list" and deprecate the later? >>>>> >>>>> The remaining question and the primary reason for this cover letter >>>>> is, should I change the kernel define for this as well? From >>>>> TIPC_NL_NODE_REMOVE to TIPC_NL_PEER_REMOVE? I'm not sure how it >>> would >>>>> fit in the current curnel space context. Note that this is not yet merged. >>>>> >>>>> Richard Alpe (1): >>>>> tipc: add peer remove functionality >>>>> >>>>> include/linux/tipc_netlink.h | 1 + >>>>> man/man8/tipc-bearer.8 | 1 + >>>>> man/man8/tipc-link.8 | 1 + >>>>> man/man8/tipc-media.8 | 1 + >>>>> man/man8/tipc-nametable.8 | 1 + >>>>> man/man8/tipc-node.8 | 1 + >>>>> man/man8/tipc-peer.8 | 52 +++++++++++++++++++++++++ >>>>> man/man8/tipc.8 | 1 + >>>>> tipc/Makefile | 2 +- >>>>> tipc/peer.c | 93 >>>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>> tipc/peer.h | 21 ++++++++++ >>>>> tipc/tipc.c | 3 ++ >>>>> 12 files changed, 177 insertions(+), 1 deletion(-) create mode >>>>> 100644 >>>>> man/man8/tipc-peer.8 create mode 100644 tipc/peer.c create mode >>>>> 100644 tipc/peer.h >>>>> >>>>> -- >>>>> 2.1.4 >>>> >> > > ------------------------------------------------------------------------------ _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
