Merge lp:~mgorven/ibid/oui into lp:~ibid-core/ibid/old-trunk-1.6

[Michael Gorven]

[marcog (marco-gallotta)]

Query: reload sysadmin
DEBUG:core.reloader:Loading Processor: sysadmin.AptFile
ERROR:core.reloader:Couldn't instantiate AptFile processor of sysadmin plugin
Traceback (most recent call last):
  File "./ibid/core.py", line 235, in load_processor
    ibid.processors.append(klass(name))
  File "./ibid/plugins/__init__.py", line 80, in __init__
    self.setup()
  File "./ibid/plugins/sysadmin.py", line 118, in setup
    raise Exception("Cannot locate apt-file executable")
Exception: Cannot locate apt-file executable
Response: Couldn't reload sysadmin

Not sure if we usually implement workarounds for this, but this is failing because another feature in this plugin has a dependency that isn't installed.

+ @match(r'^(?:(?:mac|oui|ether|ether(?:net)?(?:\s*code)?)\s+)?((?:(?:[0-9a-f]{2}[:-]?){3}){1,2})$')

You can remove the "ether|" as it's caught by the one that follows.

Do you want to allow a trailing [:-]? (e.g. 11:22:33: matches)

Query: 112233
Response: I don't know who that belongs to

That is very unexpected if the user was not trying to lookup a mac address. I would require the 11:22:33 format (even disallowing 11-22-33 because that looks like an expression) if the leading word (mac, ether, etc.) is left out.

review: Needs Fixing

[marcog (marco-gallotta)]

+ oui = mac.replace('-', '').replace(':', '').upper()[:6]

Why the [:6]?

Perhaps use re.sub to strip [-:] in a single call.

[marcog (marco-gallotta)]

+ oui = mac.replace('-', '').replace(':', '').upper()[:6]

Why the [:6]?

Perhaps use re.sub to strip [-:] in a single call.

review: Needs Fixing

[Michael Gorven (mgorven)]

> Why the [:6]?

The first 6 hex characters identify the manufacturer.

> Perhaps use re.sub to strip [-:] in a single call.

It's probably more efficient to do 2 replaces() ;-)

[Stefano Rivera (stefanor)]

> Not sure if we usually implement workarounds for this, but this is failing because another feature in this plugin has a dependency that isn't installed.

We don't, but that is probably out of scope for this review. There is a plan to rework dependency handling, but in the meantime we just hack around it.

[Keegan Carruthers-Smith (keegan-csmith)]

> Not sure if we usually implement workarounds for this, but this is failing
> because another feature in this plugin has a dependency that isn't installed.

I agree, why should everything fail just because of that? Especially since this will not be necessarily run on a debianesque machine.

> + @match(r'^(?:(?:mac|oui|ether|ether(?:net)?(?:\s*code)?)\s+)?((?:(?:[0-9a-f]
> {2}[:-]?){3}){1,2})$')
>
> You can remove the "ether|" as it's caught by the one that follows.

Agreed.

Otherwise I approve.

review: Approve

[Stefano Rivera (stefanor)]

worksforme

review: Approve

[lp:~mgorven/ibid/oui]

888. By Michael Gorven on 2010-02-13

Require that MAC address contains colons when not prefixed with keyword.

889. By Michael Gorven on 2010-02-13

Merge from trunk to resolve conflict.

[Michael Gorven (mgorven)]

> That is very unexpected if the user was not trying to lookup a mac address.
> I would require the 11:22:33 format (even disallowing 11-22-33 because
> that looks like an expression) if the leading word (mac, ether, etc.) is
> left out.

r888. It does allow anything from 3 to 6 groups, but I think that's okay.

[Max Rabkin (max-rabkin)]

> name = u' '.join(word.capitalize() for word in name.split(' '))

I think you meant "name = name.title()"

[Michael Gorven (mgorven)]

> I think you meant "name = name.title()"

Uh, of course ;-) r890

[lp:~mgorven/ibid/oui]

890. By Michael Gorven on 2010-02-13

<3 Python

[Michael Gorven (mgorven)]

r891 closes the file properly.

[lp:~mgorven/ibid/oui]

891. By Michael Gorven on 2010-02-13

Let's rather not leak file descriptors.

[marcog (marco-gallotta)]

> > That is very unexpected if the user was not trying to lookup a mac address.
> > I would require the 11:22:33 format (even disallowing 11-22-33 because
> > that looks like an expression) if the leading word (mac, ether, etc.) is
> > left out.
>
> r888. It does allow anything from 3 to 6 groups, but I think that's okay.

Getting very nitpicky, but this is a bit weird:

Query: mac 123456
Response: I don't know who that belongs to
Query: mac 1234567
Response: Excuse me?
Query: mac 12345678
Response: I don't know who that belongs to

review: Approve

=== modified file 'ibid/plugins/sysadmin.py'
--- ibid/plugins/sysadmin.py	2010-02-09 14:39:23 +0000
+++ ibid/plugins/sysadmin.py	2010-02-13 12:16:18 +0000
@@ -2,11 +2,12 @@
 # Released under terms of the MIT/X/Expat Licence. See COPYING for details.
 
 import os
+import re
 from subprocess import Popen, PIPE
 
 from ibid.plugins import Processor, match
 from ibid.config import Option
-from ibid.utils import file_in_path, unicode_output, human_join
+from ibid.utils import file_in_path, unicode_output, human_join, cacheable_download
 
 help = {}
 
@@ -181,4 +182,21 @@
             if index:
                 event.addresponse(output[index+1].strip())
 
+help ['mac'] = u'Finds the organization owning the specific MAC address.'
+class Mac(Processor):
+    u"""mac <address>"""
+    feature = 'mac'
+
+    @match(r'^((?:mac|oui|ether(?:net)?(?:\s*code)?)\s+)?((?:(?:[0-9a-f]{2}(?(1)[:-]?|:))){2,5}[0-9a-f]{2})$')
+    def lookup_mac(self, event, _, mac):
+        oui = mac.replace('-', '').replace(':', '').upper()[:6]
+        ouis = open(cacheable_download('http://standards.ieee.org/regauth/oui/oui.txt', 'sysadmin/oui.txt'))
+        match = re.search(r'^%s\s+\(base 16\)\s+(.+?)$' % oui, ouis.read(), re.MULTILINE)
+        ouis.close()
+        if match:
+            name = match.group(1).decode('utf8').title()
+            event.addresponse(u"That belongs to %s", name)
+        else:
+            event.addresponse(u"I don't know who that belongs to")
+
 # vi: set et sta sw=4 ts=4: