fix for colon split for windows paths

[ghost]

Fix for bug in arguments parsing.
On windows systems, paths will contain the colon character, which will be misinterpreted by the Agent as a separator.
This fix limit the splitting of the string to the first occurrence of the colon sign, leaving the path intact.

[brian-brazil]

This would break providing a hostname

[ghost]

The hostname has already been parsed when my change is executed. The agentArgument is reassigned to the remaining bit of the string.
I've tried running mvn test but no tests are run.
Do you have a suit of tests I can run to make sure nothing is broken?

[brian-brazil]

Only if it's a v6 IP, this doesn't have tests.

[ghost]

Hi Brian,
I've updated my PR by replacing the old parsing logic with regex patterns for the different argument combinations.
I've added some tests (currently in the JavaAgentIT class as I could not find a better fit) which address the different scenarios.
Please, take a careful look at those tests to make sure they match the intended use.
Also note, I've changed what I thought was a typo in the error message you wrote. It currently reads:
Usage: -javaagent:/path/to/JavaAgent.jar=[host]:<port>:<yaml configuration file> with the colon past the square brackets.
It would be useful in the future to document the syntax to pass arguments (did not find that anywhere in the docs).

[brian-brazil]

This is getting a bit complicated. It'd be easier to require all parameters to be set if you want to use a colon.

[brian-brazil]

Does java support port names?

[ghost]

What do you mean by port names?
If you meant group names, yes, java does support them but only starting from version 7 while the pom file in the project specifies java 6.

[brian-brazil]

http is 80 for example, from /etc/services.

[ghost]

Typically, if you use HTTP and you don't specify a port, the default one would be taken, which is 80. Same for HTTPS; 443 would be your default port if you don't specify a custom one.

I don't see how that's relevant to the arg parsing tho.

[brian-brazil]

My point is that the string http is equivalent to 80, and both are valid ports. You're only matching numbers.

[brian-brazil]

What about DNS is v4?

[ghost]

Not sure I get your question there.
The DNS_IPV4_REGEX means that regex applies to both DNS names and ipv4 addresses.

[ghost]

Hi Brian,

requiring to pass all parameters will not solve the issue as long as you blindly split on colons.
Also, that would be a breaking functional change, which I tried to avoid in my work here.

I would maintain the parsing system I'm proposing here is more rational and, importantly, isolates a parsing method that can be tested separately.
So everything one would need to worry about there is whether the tests cover the intended cases. If not, more tests can be added and, if they fail, the parsing functionality improved accordingly.

[brian-brazil]

I mean from an end-user standpoint. I'd rather not have such a complex parsing system.

[ghost]

The end user (which in our case is the user that passes the args) should not know about the parsing logic. As I was trying to explain above, the only part he/she's concerned with is how those args should be passed.
As I mentioned earlier, the correct syntax for the args was already defined before my edit and I did not change it. Namely, that system defined 3 ways of passing args:

1. [ipv6]:port:file
2. ipv4/dns:port:file
3. port:file

So to your point, the parsing system is no concern for the end user.

If you say you'd rather have a different parsing system, can you please specify what that should look like?

[brian-brazil]

I'm proposing that if you want to use a colon in the file, you must specify the host.

[ghost]

About your proposal to force the user to specify a host if they want to pass a path that contains colons, that alone will not solve the issue.
The issue lies here: String[] args = agentArgument.split(":"); (line 32 in JavaAgent)
So:

• for dns/ipv4, forcing the user to pass hostname does not solve the problem. Namely, if you pass something like my.server.com:4407:C:\Program Files\settings.yml your code will fail when parsing this part 4407:C:\Program Files\settings.yml because it will split over the colon inC:\.

• for ipv6, nothing changes as the ipv6 part is removed before the port and file are parsed.

So the parsing logic of what happens needs to be changed there in any case.

[ghost]

If all you want is a simple solution and you don't mind changing the parameters syntax i propose:

• forcing the user to always pass the hostname in square brackets, even when ipv4 or dns
• implementing my original suggestion, that is, limiting the splitting to the first colon with String[] args = agentArgument.split(":", 2);

[ghost]

Hi Brian, any updates on this?
Is my proposal clear?
Do you need any clarifications?

[brian-brazil]

Obsoleted by #371