Ok. You evidently have spent more time than me pondering about this, so I have no more objections.
///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
