-
Notifications
You must be signed in to change notification settings - Fork 26
make multiaddr-net more pluggable #15
Changes from 3 commits
347065f
1263064
48f060e
79cf06b
b730d26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package manet | ||
|
||
import ( | ||
"fmt" | ||
"net" | ||
"sync" | ||
|
||
ma "github.com/jbenet/go-multiaddr" | ||
) | ||
|
||
type AddrParser func(a net.Addr) (ma.Multiaddr, error) | ||
type MaddrParser func(ma ma.Multiaddr) (net.Addr, error) | ||
|
||
var maddrParsers map[string]MaddrParser | ||
var addrParsers map[string]AddrParser | ||
var addrParsersLock sync.Mutex | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. woah please no static stuff like this. I know it's common practice in "i'm doing it live" Go, but this is why we cant have nice things. Users may extend multiaddr with different addrs clashing on a key, and suddenly magically everything could break on them. Static monkey patching like this is pretty bad practice in highly modular code bases. with something like libp2p, we're bound to run into a problem. this is similar, but actually way more flexible: type CodecMap struct{
codecs map[string]NetCodec
sync.Mutex
}
func (cm *CodecMap) RegisterAddressType(a *AddressSpec) {
cm.Lock()
defer cm.Unlock()
for _, k := range a.NetNames {
cm.codecs[k] = a.NetCodec
}
cm.codecs[a.Key] = a.NetCodec
}
var defaultCodecs CodecMap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the reason this is better is that this allows "those who want to be careful with modularity" to be perfectly safe, while still making the defaults do the typical go expected behavior for most simple programs. |
||
|
||
type AddressSpec struct { | ||
// NetNames is an array of strings that may be returned | ||
// by net.Addr.Network() calls on addresses belonging to this type | ||
NetNames []string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. call this |
||
|
||
// Key is the string value for Multiaddr address keys | ||
Key string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont understand what this means. is this the default one of the names above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh i see. it's not called a "Key" nor "address keys". it's called a "Protocol Name". https://github.com/jbenet/multiaddr/blob/master/protocols.csv and https://github.com/jbenet/go-multiaddr/blob/master/protocols.go#L13 call this property |
||
|
||
// ParseNetAddr parses a net.Addr belonging to this type into a multiaddr | ||
ParseNetAddr AddrParser | ||
|
||
// ConvertMultiaddr converts a multiaddr of this type back into a net.Addr | ||
ConvertMultiaddr MaddrParser | ||
|
||
// Protocol returns the multiaddr protocol struct for this type | ||
Protocol ma.Protocol | ||
} | ||
|
||
func RegisterAddressType(a *AddressSpec) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a test for this func would be awesome -- then LGTM. 🐴 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its tested indirectly. None of the tests would function if this function didnt work right :P There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i did add a small test for this directly |
||
addrParsersLock.Lock() | ||
defer addrParsersLock.Unlock() | ||
for _, n := range a.NetNames { | ||
addrParsers[n] = a.ParseNetAddr | ||
} | ||
|
||
maddrParsers[a.Key] = a.ConvertMultiaddr | ||
} | ||
|
||
func init() { | ||
addrParsers = make(map[string]AddrParser) | ||
maddrParsers = make(map[string]MaddrParser) | ||
|
||
RegisterAddressType(tcpAddrSpec) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense for the default specs to live in here rather than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hrm... you might be right there. code organization is hard |
||
RegisterAddressType(udpAddrSpec) | ||
RegisterAddressType(utpAddrSpec) | ||
RegisterAddressType(ip4AddrSpec) | ||
RegisterAddressType(ip6AddrSpec) | ||
|
||
addrParsers["ip+net"] = parseIpPlusNetAddr | ||
} | ||
|
||
func getAddrParser(net string) (AddrParser, error) { | ||
addrParsersLock.Lock() | ||
defer addrParsersLock.Unlock() | ||
|
||
parser, ok := addrParsers[net] | ||
if !ok { | ||
return nil, fmt.Errorf("unknown network %v", net) | ||
} | ||
return parser, nil | ||
} | ||
|
||
func getMaddrParser(name string) (MaddrParser, error) { | ||
addrParsersLock.Lock() | ||
defer addrParsersLock.Unlock() | ||
p, ok := maddrParsers[name] | ||
if !ok { | ||
return nil, fmt.Errorf("network not supported: %s", name) | ||
} | ||
|
||
return p, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of these two functions, which are named a bit confusingly, either: