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/iplink.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
