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