Re: [PATCH] validate -n parameter value

On 16/07/2014 10:53 a.m., Alex Rousskov wrote:
> 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.

Ouch. Fixed now.

Amos
Received on Fri Jul 18 2014 - 11:52:42 MDT