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

Improve internal message routing #62

Open
samrussell opened this issue Nov 10, 2018 · 4 comments
Open

Improve internal message routing #62

samrussell opened this issue Nov 10, 2018 · 4 comments

Comments

@samrussell
Copy link
Collaborator

There's a lot of the following pattern:

if isinstance(x, Class1):
  do_something(x)
elif isinstance(x, Class2):
  do_something_else(x)

This started as a quick way to get a prototype up and running but we need to find a better pattern for this. Python isn't the nicest language for this, but even moving to a dictionary of {class: method} would be an improvement

@Bairdo
Copy link
Contributor

Bairdo commented Nov 12, 2018

Agreed.
With the dict way, how would you deal with methods that have different arguments?
e.g. https://github.com/faucetsdn/chewie/blob/master/chewie/message_parser.py#L243

BTW - Currently experimenting with a generic EAP class that that will replace the EAP method classes (TLS, TTLS, MD5, PEAP, ...)

@samrussell
Copy link
Collaborator Author

That was part of the reason for making EapolXMessage style classes - you just pass the object to the appropriate handler. This actually ends up being easier in strongly typed OOP languages, you can use visitor patterns (or in C#, dynamic dispatch) and it all collapses into a single line.

As for the generic EAP class, generics aren't always a good thing. The whole Don't Repeat Yourself (DRY) mantra in software is a useful rule of thumb, but it's only that - the downside is that once you do that you've committed to an abstraction, so you should do so only once you're ready to commit.

As with #51, the goal isn't to get full feature support on day 1, the goal is to get this running on a real network so we can start getting real bugs that only turn up live, with users doing weird things and different vendor interop. Once we've got it deployed somewhere we'll know where to put our attention next.

To put this another way, if VUW want to deploy this and the only thing holding them back is MSCHAP support then we should totally build that in - that would be a step towards a live deployment

@Bairdo
Copy link
Contributor

Bairdo commented Nov 12, 2018

Generic might have been the wrong word, it's a class that treats all of TTLS, TLS, MD5, and any other methods the same. The packet format is the same for the first 5 bytes, then after that chewie doesn't need to know the exact details. I'll push it up shortly.

@Bairdo
Copy link
Contributor

Bairdo commented Nov 12, 2018

fyi pr #71

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

No branches or pull requests

2 participants