Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

[v0x02] Review the use of "Type" #154

Closed
diraol opened this issue Sep 2, 2016 · 9 comments
Closed

[v0x02] Review the use of "Type" #154

diraol opened this issue Sep 2, 2016 · 9 comments
Assignees
Milestone

Comments

@diraol
Copy link
Contributor

diraol commented Sep 2, 2016

As we have changes on the Type class (ofp_type) values, we need to review all classes that have had it's types changed on that class.

For example, on the v0x01 version, Type.PORT_MOD value is 15, but on the v0x02 Type.PORT_MOD value is 16, while Type(15) is GROUP_MOD.

So, for all Messages (from GenericMessage) we need to check if its Type has changed on the enum. If it has, then we need to fix it on the message implementation, making sure that the Type is right according to v0x02.

@cemsbr cemsbr self-assigned this Sep 2, 2016
@cemsbr
Copy link
Contributor

cemsbr commented Sep 2, 2016

I believe Type is one case of many others. We should convert all values to names.

@diraol
Copy link
Contributor Author

diraol commented Sep 5, 2016

@cemsbr we can have problems with inheritance on that matter.

I'll try to do an example.

On v0x01 we have the Hello message.

It is mainly a header with header.message_type == Type.HELLO, right?

Well, on the v0x02, the Hello message is exactly the same based on this idea.

But, it on v0x01 Type.HELLO have the value of 0and on v0x02 Type.HELLO have the value of 1, then on v0x02 we can't just inherit the Hello message from v0x01. Despite the fact that we are using the name on the class definition.

So, with that, may be we will need to reimplement all messages that the integer value of the "Type" have changed between of versions. Does it seems right for you?

@diraol
Copy link
Contributor Author

diraol commented Sep 6, 2016

I think that this issue is related to #157 .
If you guys agree, then may be we can close this issue and let the work for that one.

@cemsbr
Copy link
Contributor

cemsbr commented Sep 6, 2016

Yes, I think re-implementation is the way to go. We should be able to use Enum's field instead of integers.

@diraol
Copy link
Contributor Author

diraol commented Sep 6, 2016

Actually using the "enum field" does not fix the problem.
For example:

v0x01

>>>class Type_v0x01(Enum):
>>>  HELLO = 0
>>>  ERROR = 1
>>>  ECHO_REQUEST = 2
>>>  ECHO_REPLY = 3

>>>class Type_v0x02(Enum):
>>>  HELLO = 0
>>>  ERROR = 1
>>>  KYTOS = 2
>>>  ECHO_REQUEST = 3
>>>  ECHO_REPLY = 4

>>>Type_v0x01.ECHO_REPLY
3
>>>Type_v0x02.ECHO_REPLY
4

The problem still remains.... the reference will always be to the 'older' version.

@cemsbr
Copy link
Contributor

cemsbr commented Sep 6, 2016

But if Hello is re-implemented importing v0x02's Type and then we import v0x02's Type when unpacking, there's no problem, right? The unpacking algorithm can remain the same if it uses Enum names instead of values.

@diraol
Copy link
Contributor Author

diraol commented Sep 6, 2016

Yes! If we re-implement the class there is no problem.

@beraldoleal beraldoleal changed the title Review the use of "Type" on v0x02 [v0x02] Review the use of "Type" Oct 1, 2016
@beraldoleal beraldoleal added this to the 2017.1b2 milestone Mar 31, 2017
@beraldoleal
Copy link
Member

Type_V0X01 is terrible!

@diraol
Copy link
Contributor Author

diraol commented Mar 31, 2017

@beraldoleal This issue can be closed in favor of #157. Even if we do not change the 'inheritance model', the Type class will be defined within the version module, so this problem will not happen.

@cemsbr do you agree?

@diraol diraol closed this as completed Mar 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants