Re: [PATCH] validate -n parameter value

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
Received on Mon Jun 16 2014 - 16:07:01 MDT