Re: [PATCH] validate -n parameter value

On 07/14/2014 07:02 AM, Amos Jeffries wrote:
> On 23/06/2014 12:35 a.m., Amos Jeffries wrote:
>> 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.

> No can do on removal. Without it we get:
> error: request for member 'prefix' in 'tok', which is of non-class type
> 'Parser::Tokenizer(SBuf)'

Yeah, we appear to hit some hairy C++ concept here that I cannot fully
reconstruct. I could not find an explanation by quick googling (all
results point to trivial cases of declaring a function instead of
calling the default constructor).

The following works for me:

  ::Parser::Tokenizer tok(SBuf(optarg, SBuf::npos));

However, at this point, it is not clear whether the extra "t" temporary
is actually worse than the extra SBuf argument! If you keep "t", a more
descriptive name for t would be nice but obviously not required.

>>>> + 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.
>>>
>
> Moved this to an explicit check on *optarg content so it is obviously
> intentional and gets the right "missing service name" error.

The adjusted if condition in the committed code appears to dereference a
nil pointer when the service name is missing:

> if (optarg || *optarg == '\0') {

The code actually does not get that far because getopt() does not allow
a missing service name (at least in my tests), but the condition should
be fixed.

HTH,

Alex.
Received on Tue Jul 15 2014 - 22:53:29 MDT