Bug 2447

Summary: Attribute system doesn't complain in case of errors.
Product: ns-3 Reporter: Tommaso Pecorella <tommaso.pecorella>
Component: coreAssignee: 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
The problem was outlined here:
https://groups.google.com/forum/#!topic/ns-3-users/pq5xGa4khBc

Basically the problem is: stuff like
clientHelper.SetAttribute ("OnTime", StringValue ("ns3::UniformRandomVariable[Min=0.,Max=1.]"));

doesn't raise any warning or error.

The problem is: the tokenization is based on a "|" character. As a consequence, it doesn't happens.
The UniformRandomVariable class is initialized with a "Min" Attribute set to "0.,Max=1."

Shouldn't this be considered an error by the attribute system ?
Comment 1 Tom Henderson 2016-06-22 20:52:33 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.
Comment 2 Tommaso Pecorella 2016-06-23 17:22:51 EDT
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.
Comment 3 Tommaso Pecorella 2016-06-23 18:02:26 EDT
One more thing.

I checked. The Attribute syntax with Brackets it is *not* documented.
We must add this point to the manual.
Comment 4 Tom Henderson 2016-09-12 15:33:28 EDT
Created attachment 2576 [details]
patch to fix

extends Tommaso's 'different approach' patch
Comment 5 Tom Henderson 2016-09-12 15:34:07 EDT
Created attachment 2577 [details]
example program

Example program to test the code
Comment 6 Tom Henderson 2016-09-12 15:38:06 EDT
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
Comment 7 Tommaso Pecorella 2016-09-12 15:42:18 EDT
+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
Comment 8 Peter Barnes 2016-09-12 19:20:57 EDT
+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}]")
Comment 9 Tom Henderson 2016-09-26 23:43:10 EDT
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