On 06/15/2014 12:05 AM, Amos Jeffries wrote:
> + if (optarg) {
> + SBuf t(optarg);
> + ::Parser::Tokenizer tok(t);
Pleaser remove "t" as used-once and lacking a descriptive name. If you
insist on keeping it, please make it "const" and try to give it a more
descriptive name such as rawName or serviceName.
> + const CharacterSet chr =
> CharacterSet::ALPHA+CharacterSet::DIGIT;
Is there a document stating that only those characters are valid? My
quick search resulted in [1] that seems to imply many other characters
are accepted. However, [2] lists more prohibited characters. Both seem
to allow "-" and "_" though. It would be good to provide a reference in
the code or commit message to explain why we are restricting its value.
Do you want to validate the total service name length? [1] says the
limit is 256 characters.
None of the above applies to -n used on Unix. Do we need to validate the
service name (whatever that is) there?
> + if (!tok.prefix(service_name, chr) || !tok.atEnd())
Please note that the above also rejects empty service names (-n "").
That may be good, but the error messages do not reflect that restriction.
> + fatal("Need to provide alphanumeric service name when
> using -n option");
> + opt_signal_service = true;
> + } else {
> + fatal("Need to provide alphanumeric service name when using
> -n option");
> + }
I recommend adjusting the fatal() messages to distinguish the two errors
and avoid a possible misunderstanding that the service name was provided
but was not alphanumeric in the second case:
* Expected alphanumeric service name for the -n option but got: ...
* A service name is required for the -n option.
I continue to dislike the undefined -n option meaning on non-Windows
boxes, but I have no objection to this patch and believe the above
changes can be done without a new review cycle.
Cheers,
Alex.
[1] http://msdn.microsoft.com/en-us/library/ms682450%28VS.85%29.aspx
[2] http://www.anzio.com/resources/cannot-install-or-start-windows-service
