[WIP] Portname validation

[damn1]

While fixing YARP crashes on wrong inputs, after #1470, I tried to define a whitelist of accepted and correctly parsed inputs.
So I would like to propose this PR as an attempt to standardize YARP port names and to start to supply (hopefully) clear documentation about yarp::os::NestedContact and other ROS related classes ( #1501).
Consists of adding two functions and calling them on input port names to Port::open() and Network::connect().

These two functions together should define the policies to define port names. There should be anything new, I just checked what YARP supports and tried to formalize the input formats accepted.
To define the allowed syntax I checked the code in Port::open(), Contact::fromString() and NestedContact::fromString(), besides the port names used in the tests as a reference for common names.
Let me know if this could be useful or not.

---

So here are the names accepted on registration:

• "" - blank name is accepted to open anonymous ports.
• some_topic_name - no leading slash, this name is accepted and then discarded if there's no active node.
• ... - this is assigned a temporary port in the sequence /tmp/port/N.

Following syntaxes are accepted both as registration and registered names, and checked by Network::connect()

• carrier:/port[cat]@node~wire
• carrier:/node[cat]#port~wire
• carrier:/node=[cat]port~wire
• carrier:/machine:NNN/

Clearly carriers, nodes, wires and category are optional.

Syntax rules for ports and nodes:

• leading '/'
• only letters and numbers
• other whitelisted characters are '_' and ':' to define readable ports like /p_1 or /port:someinfo
• the pattern is repeatable /p1/p2/p3:s1/p_4/p_5_and_6:s2
• '_' and ':' cannot be terminating and leading chars after a '/'

Syntax rules for category ([cat]):

• Possible and mutually exclusive categories are +, -, +1, -1.

Syntax rules for carriers and wires:

• letters, numbers and '_' without a specific pattern.
  ~wire is used to specify the type of the communication on topics.

Syntax rules for machines:

• leading '/'
• letters and numbers
• followed by '.' and some other letters and numbers (repeatable pattern)
• followed by ':' and port number
• optional trailing '/'

Behaviour:
The '=', '#' symbols should be equivalent but the position of the category.
'@' syntax is still equivalent with inverted position of node and topic.
Registration names assumed by ports with these syntaxes could be different if you specify or not the category (#1470 (comment))

One explicit difference with this patch is that @ allows "" as node name (a node like /tmp/port/N is assigned)
while node can't be blank with

• '#' since in command yarp read #/port #/port is taken as bash comment.
• '=' since the behaviour is widely broken (Aborted core opening ports starting with "=" #1470)

Error reporting on wrong matches is very basic but in many cases could be useful, if a part of the expression is matched:

"/p1/someInvalidNam&/p4" does not match the correct pattern for port definition.
Check for the error in prefix or suffix of the matched name:
/p1/someInvalidNam [&/p4]

[traversaro]

I think that all this nice information about the allowed port names (basically the description of the PR after ---) should go in an appropriate documentation page such as https://github.com/robotology/yarp/blob/master/doc/yarp_env_variables.md , that can be linked from the isValidRegisteredName and isValidRegistrationName doxygen documentation method.

[traversaro]

Related issues and PRs :

• problem connecting unnamed ports via topic #169
• Add extra check during the connect #1441 (comment)

[traversaro]

Just out of curiosity, do you have any idea about the order of magnitude of the time spent on performing the regex ?

[pattacini]

Seems you carried out quite a deep investigation @damn1
I'm not sure if I've caught all the underlying motivations and implications for this standardization, but I'm wondering whether /port-name is still an allowed port name.
If not, why?

[traversaro]

Given that this is an error, perhaps it is better to use std::cerr or one of the countless logging mechanism that can be used in libYARP_OS ?

[traversaro]

I do not fully understand this message.

[traversaro]

Do you have any idea about the order of magnitute necessary to build the std::regex object? If it is not trivial, we could think about having a regex singleton to avoid rebuilding the regex for each check.

[barbalberto]

Thanks @damn1 for this thorough test.
I think we have now a solid knowledge to define what we want to be accepted as a valid port name and what sould not.

I'd propose a pit fight ... I mean, a meeting, to write down a rule set and publish it online as @traversaro suggested.

@pattacini i think /port-name is accepted, but /port-@name may be interpreted by yarp as a ROS topic (I'm not sure)

[damn1]

@traversaro for sure if this PR is going to be accepted I would keep the task of report the documentation in the proper place. About the time of parsing I actually have no idea at the moment. I can try to collect some info about that.
@pattacini /port-name is not valid 😓 just because I didn't think about it and there were no port like that in the tests. But of course it would not be a problem to add it and to whitelist some other characters if someone thinks so.
@barbalberto agree for the meeting, as I think the whitelist should be approved and still the PR leaves some open problems (in my opinion 3 syntaxes to do the same thing but with different names result could be very misleading)

[pattacini]

@damn1 yep the syntax /port-name is used indeed and therefore, please add it to the whitelist 👍
I cannot think of other similar symbols as of now.

Take home message

Standardization is quite a nice tool to make a product more mature. However, when we want to standardize a software that is already used since years, standardization becomes somewhat difficult. That's exactly the reason why we should call for a meeting as @barbalberto said since a PR alone is not enough (I might have missed it) to elicit the number of observations and suggestions required to keep the error probability low 😉

[damn1]

@pattacini I fully understand the necessities of a meeting, I made a PR just because I had some work done and wanted to make it public to see if there's interest in this question, and if it was being done in a correct way (technically speaking), otherwise I would drop the work or change direction.
As you can see it's a rough and unassuming work and there are many things to be reviewed as @traversaro already pointed out. It's more a proposal than a pull request, but I didn't know other way to expose it.

[pattacini]

Clear, thanks @damn1
I didn't have the info to correctly judge this PR as a proposal 👍

[drdanz]

+1 for the meeting
Also I would like to see a massive unit test for this change, testing all patterns and all the possibilities, including nodes, publisher, subscribers, topics, YARP_PORT_PREFIX and YARP_RENAME_.

[drdanz]

As discussed with @damn1, I will close this and add the Type: Reminder label for now