Bugzilla – Full Text Bug Listing |
Summary: | Attribute system doesn't complain in case of errors. | ||
---|---|---|---|
Product: | ns-3 | Reporter: | Tommaso Pecorella <tommaso.pecorella> |
Component: | core | Assignee: | Peter Barnes <pdbarnes> |
Status: | PATCH WANTED --- | ||
Severity: | enhancement | CC: | ns-bugs, tomh |
Priority: | P5 | ||
Version: | ns-3-dev | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://www.nsnam.org/bugzilla/show_bug.cgi?id=2446 https://www.nsnam.org/bugzilla/show_bug.cgi?id=2815 |
||
Attachments: |
possible patch
different approach patch to fix example program |
Description
Tommaso Pecorella
2016-06-22 18:00:36 EDT
Created attachment 2478 [details]
possible patch
Attached patch will screen for invalid delimiter characters of ',' or ':'.
A fuller patch could document all of the illegal characters for within a StringValue and do a more complete search, and search for all such characters.
Another possibility is to allow ',' optionally in addition to '|'; thoughts?
We should decide first on whether '|' is the only legal delimiter and whether we want to be more strict about input string validation.
Created attachment 2483 [details] different approach I thought to do a check there too. However it will never be complete. A slightly better idea (in my opinion) is to move the check in the deserialization. If the tokenization if done properly, then the stream that the AttributeValue is parsing will be fully consumed (eof bit set). If eof is not set, then there's a mistake somewhere. Note that space aren't allowed in the attributes values anyway, so there's no need to check for that. This patch implements this, and catches the commas. As a side note, I'm fine with allowing both | and comma. It seems that commas are more "natural", especially for the ones used to LaTeX, ObjectiveC, Swift, etc. (In reply to Tom Henderson from comment #1) > Created attachment 2478 [details] > possible patch > > Attached patch will screen for invalid delimiter characters of ',' or ':'. > > A fuller patch could document all of the illegal characters for within a > StringValue and do a more complete search, and search for all such > characters. > > Another possibility is to allow ',' optionally in addition to '|'; thoughts? > > We should decide first on whether '|' is the only legal delimiter and > whether we want to be more strict about input string validation. One more thing. I checked. The Attribute syntax with Brackets it is *not* documented. We must add this point to the manual. Created attachment 2576 [details]
patch to fix
extends Tommaso's 'different approach' patch
Created attachment 2577 [details]
example program
Example program to test the code
After offline discussion with Peter, I propose to commit the attached two patches (after Tommaso and Peter approve) to fix the immediate problem of silent failures due to invalid delimiters. The example program can be used to confirm the old incorrect behavior and new correct behavior, and can be referenced by future documentation on how StringValues can be used in object factories and attributes. The example program is also hooked to test.py. I propose to leave this issue open after committing, until the following remaining issues are completed: 1) documentation of the attribute setting syntax 2) more permissive syntax to allow a comma delimiter +1, the priority is to catch the unexpected behaviour. T. (In reply to Tom Henderson from comment #6) > After offline discussion with Peter, I propose to commit the attached two > patches (after Tommaso and Peter approve) to fix the immediate problem of > silent failures due to invalid delimiters. The example program can be used > to confirm the old incorrect behavior and new correct behavior, and can be > referenced by future documentation on how StringValues can be used in object > factories and attributes. The example program is also hooked to test.py. > > I propose to leave this issue open after committing, until the following > remaining issues are completed: > 1) documentation of the attribute setting syntax > 2) more permissive syntax to allow a comma delimiter +1. I've been working on a patch to replace all StringValue ("ns3::...") with MakeTypeIdValue<...> (). I'll try and extend that to check the TypeId name in constructions like StringValue ("ns3::{name}[{attribute string}]") patch (changeset 12334:088c38db7724) and example (12335:5b84ce8689d1) pushed; leaving bug open for: 1) documentation of the attribute setting syntax 2) more permissive syntax to allow a comma delimiter |