Re: Patch for squidclient

Markus Moeller wrote:
>
> "Alex Rousskov" <rousskov_at_measurement-factory.com> wrote in message
> news:4C67F515.6080303_at_measurement-factory.com...
>> On 08/14/2010 02:10 PM, Markus Moeller wrote:
>>
>>> Please find attached a patch to add Proxy- and WWW-Authenticate.
>>
>> * GSSAPI_token not documented.
>>
>> * check_gss_err not documented.
>>
>
> I did not see any function with documentation. I have added some lines
> now. What should be the format ?

Doxygen please:

/**
  * description...
  *
  * \retval 1 gssapi error
  * \retval 0 successful, no gssapi error.
  */

>
>> * It would be nice to remove gotos from the new code.
>>
>
> Done
>
>> * porxy misspelled; did not check for other typos
>>
>
> Fixed
>
>> * Please try to remove whitespace modifications that are unrelated to
>> your patch.
>>
>
> I used formater.pl, which must have introduced them.

>
>> * Is tools/Makefile.in under revision control? If not, it should not
>> be in the patch.
>>
>
> Not sure if it is under revision control, but I get it with rsync. I
> have removed it from the patch
>

The -h help text is mean to list the options in alphabetical order.

Also on the Usage: line. You can split at -m and -p like so:
    "[-k] [-l local-host] [-m method] "
+#if HAVE_GSSAPI
+ "[-n] [-N] "
+#endif
    "[-p port] [-P file] [-t count] [-T timeout] [-u proxy-user] [-U
www-user] "

Please update the src/tools/squidclient.1 manual page with the new options.

The "if (www_neg || proxy_neg)" around separate if for each case is
redundant.

In check_gss_err please use snprintf instead of sprintf.
  Use of a #define'd buffer size comes in handy here to replace
sizeof(buf) and calculate with when needing
   ie snprintf(buf+len, BUFFER_SIZE-len, "%s"

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.6
   Beta testers wanted for 3.2.0.1