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

Always_Valid aspect unsupported #624

Closed
treiher opened this issue Mar 29, 2021 · 4 comments · Fixed by #630
Closed

Always_Valid aspect unsupported #624

treiher opened this issue Mar 29, 2021 · 4 comments · Fixed by #630
Assignees
Labels
bug pyrflx Related to pyrflx package (Legacy Python API)

Comments

@treiher
Copy link
Collaborator

treiher commented Mar 29, 2021

% tools/validate_spec.py -s dhcp.rflx -m DHCP::Message -v tests/data/dhcp/message/valid
tests/data/dhcp/message/valid/DHCP_MessageType_10,11,12_and_13_4.raw: FAILED
FalseNegative
pyrflx: error: cannot set value for field Hlen
pyrflx: error: Number 0 is not a valid enum value

Traceback (most recent call last):
  File "tools/validate_spec.py", line 334, in <module>
    sys.exit(cli(sys.argv))
  File "tools/validate_spec.py", line 82, in cli
    validation_main(args)
  File "tools/validate_spec.py", line 135, in validation_main
    validation_result = validator.validate_message(msg)
  File "tools/validate_spec.py", line 200, in validate_message
    parser_result = self.__parse_message(original_message.bytes)
  File "tools/validate_spec.py", line 228, in __parse_message
    pdu_model.parse(message)
  File "/home/tr/sync/RecordFlux-specifications/venv/lib/python3.7/site-packages/rflx/pyrflx/typevalue.py", line 732, in parse
    ) = set_field_without_size(current_field_name, current_field)
  File "/home/tr/sync/RecordFlux-specifications/venv/lib/python3.7/site-packages/rflx/pyrflx/typevalue.py", line 712, in set_field_without_size
    self.set(field_name, value[current_pos_in_bitstring:])
  File "/home/tr/sync/RecordFlux-specifications/venv/lib/python3.7/site-packages/rflx/pyrflx/typevalue.py", line 820, in set
    field.typeval.parse(value)
  File "/home/tr/sync/RecordFlux-specifications/venv/lib/python3.7/site-packages/rflx/pyrflx/typevalue.py", line 447, in parse
    nested_message.parse(value, check)
  File "/home/tr/sync/RecordFlux-specifications/venv/lib/python3.7/site-packages/rflx/pyrflx/typevalue.py", line 726, in parse
    current_field = self._fields[current_field_name]
KeyError: ''

The used specification and test data can be found on the issue_2 branch of RecordFlux-specification.

@treiher treiher added bug pyrflx Related to pyrflx package (Legacy Python API) labels Mar 29, 2021
@rssen
Copy link
Contributor

rssen commented Mar 30, 2021

The message occurs because dhcp-auth_0.raw sets 66 as the Code. 66 is not a defined value for the Code field but since the Always Valid aspect is defined, no error is raised while setting the value to the field and UNKOWN is set as the enum literal. PyRFLX works with the literals, not the numeric values and since no literal is defined for the numeric value (66), UNKNOWN is set.

When trying to set the next field Code, the outgoing conditions are checked and none of them can be simplified to TRUE. That is because Code is UNKNOWN and the simplified method substitutes Code in the condition with it's literal. That leads to the conditions UNKNOWN = 0 or UNKNOWN = 255 and Code /= Pad_Option and Code /= End_Option. None of them can ever be TRUE. Because no condition is TRUE an empty string is returned and triggers the exception.

@treiher
Copy link
Collaborator Author

treiher commented Mar 30, 2021

So this means PyRFLX does not support the Always_Valid aspect. I suppose the current design (i.e. reusing setters for the parser) makes it hard to fix that. Or do you see a simple solution for this issue?

@treiher treiher changed the title KeyError during parsing Always_Valid aspect unsupported Mar 30, 2021
@rssen
Copy link
Contributor

rssen commented Apr 1, 2021

The main problem is that UNKNOWN is not substituted with a numeric value before calling simplified(). A solution might be to simply add UNKNOWN and the associated value to the MessageValue.type_literals or simplified_mapping (would be a better fit in type_literals) once the value is assigned. That would probably be a hack/special case in the set method of MessageValue.

@treiher
Copy link
Collaborator Author

treiher commented Apr 1, 2021

If it's that easy, we should try it.

rssen added a commit that referenced this issue Apr 1, 2021
rssen added a commit that referenced this issue Apr 1, 2021
rssen added a commit that referenced this issue Apr 1, 2021
rssen added a commit that referenced this issue Apr 1, 2021
rssen added a commit that referenced this issue Apr 8, 2021
rssen added a commit that referenced this issue Apr 8, 2021
rssen added a commit that referenced this issue Apr 8, 2021
@rssen rssen closed this as completed in #630 Apr 8, 2021
rssen added a commit that referenced this issue Apr 8, 2021
rssen added a commit that referenced this issue Apr 8, 2021
@treiher treiher mentioned this issue Aug 4, 2021
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pyrflx Related to pyrflx package (Legacy Python API)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants