On 17/06/2014 4:06 a.m., Alex Rousskov wrote: > 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. > Arbitrary design choice for guaranteed portability. Other characters can be added if necessary, but most lack cross-platform usability for all the situations the service name string is being used. > 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? IMO portable consistency is better for this type of thing than being pedantically accepting for the OS-specific character sets. Yes, length needs to be checked. Thank you. Does an arbitrary 32 bytes seem acceptible since this is a prefix on the UDS sockets and paths which the final total still needs to fit within the 256 or so for some OS? > >> + 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. > "" is not an alphanumeric string. But okay. > > 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 >