Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ipNetToMediaPhysAddress (ARP table) #19

Merged
merged 6 commits into from
Mar 28, 2017

Conversation

qiluo-msft
Copy link
Contributor

No description provided.

@@ -143,12 +143,12 @@ def size(self):
return 4 + self.length + util.pad4(self.length)

def __str__(self):
return self.string.decode('ascii')
return self.string.decode('utf-8')
Copy link
Contributor

@pavel-shirshov pavel-shirshov Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix it and add test case.


In reply to: 108312790 [](ancestors = 108312790)

@@ -98,3 +98,10 @@ def mac_decimals(mac):
"""
return tuple(int(h, 16) for h in mac.split(":"))

def ip2tuple(ip):
Copy link
Contributor

@pavel-shirshov pavel-shirshov Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipv4 only? probably it's better to use ip2tuple_v4 #Resolved

OIDs are 1-based, interfaces are 0-based, return the 1-based index
Ethernet N = N + 1
"""
match = re.match(SONIC_ETHERNET_RE_PATTERN, if_name)
Copy link
Contributor

@pavel-shirshov pavel-shirshov Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it's better to compile this regexp in init?
Also is it possible to extract this value from string using something like
if_name.replace('Ethernet', '') ? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it for readability and optimize if we find it on critical path. re library has the opportunity to hide the optimization without users changing code.


In reply to: 108313222 [](ancestors = 108313222)

if if_index is None: continue

mactuple = mac_decimals(mac)
machex = ''.join([chr(b) for b in mactuple])
Copy link
Contributor

@pavel-shirshov pavel-shirshov Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

machex = ''.join(chr(b) for b in mactuple) should work for you #Resolved

mactuple = mac_decimals(mac)
machex = ''.join(chr(b) for b in mactuple)
# if MAC is all zero
#if not any(mac): continue
Copy link
Contributor

@pavel-shirshov pavel-shirshov Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this line? What to do if the mac is 0? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct behavior of incomplete ARP entry (an IP without MAC) is not clear in the protocol. ref: http://cric.grenoble.cnrs.fr/Administrateurs/Outils/MIBS/?oid=1.3.6.1.2.1.4.22.1.2
Just keep the code here for further refinement, if needed.


In reply to: 108499363 [](ancestors = 108499363)

self.arp_dest_map[subid] = machex
self.arp_dest_list.append(subid)

# print(subid, dev, mac, ip)
Copy link
Contributor

@pavel-shirshov pavel-shirshov Mar 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove it? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


In reply to: 108499446 [](ancestors = 108499446)

@qiluo-msft qiluo-msft merged commit c0e022a into sonic-net:master Mar 28, 2017
@qiluo-msft qiluo-msft deleted the qiluo/arptable branch April 17, 2017 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants