On Dec 30, 2008, at 3:44 PM, Dan Fandrich wrote:

> On Tue, Dec 30, 2008 at 01:20:46PM -0800, Chad Monroe wrote:
>> Below is my first attempt at a patch for option negotiation. I have
>> not written
>> any test cases for it, but have tested each option in my own app
>> which I use to
>> download files (RRQ). I have not tested upload (WRQ) but support is
>> there for
>> it and it should use the same code path, however I'd recommend we
>> test it. Some
>> misc. notes regarding the fix:
> [...]
>> My one main concern is in the tftp_parse_option_ack() function. Am
>> I correct in
>> returning CURLE_TFTP_ILLEGAL in the cases where the option ACK is
>> malformed or
>> values in the option ACK are out of range?
>
> The other option would be to log a warning and just continue as
> though the
> server doesn't support option negotiation.
>
> Here are a few comments on the patch.
>
>> +.SH TFTP OPTIONS
>> +.IP CURLOPT_TFTPBLKSIZE
>> +Specify block size to use for TFTP data transmission. Valid range
>> as per
>> +RFC 2348 is 8-65464 bytes. The default of 512 bytes will be used
>> if this
>> +option is not specified.
>
> This should probably mention that use of this block size is dependent
> on support by the remote server.
>
>> +static const char *tftp_option_get(const char *buf, int len, const
>> char
>> **option, const char **value)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < len && buf[i]; i++);
>
> This idiom is confusing to read and will produce compiler warnings
> on some
> compilers. At the very least, the semicolon should be moved to a new
> line
> and a comment added that the body is empty. Better yet would be to
> simply
> use the optimized function strlen (or strnlen).
>
>> +
>> + if (i == len)
>> + return NULL;
>> + *option = buf;
>> +
>> + for (i++; i < len && buf[i]; i++);
>> +
>> + if (i == len)
>> + return NULL;
>> + *value = &buf[strlen(*option)+1];
>> +
>> + return &buf[i+1];
>> +}
>> +
>> +static CURLcode tftp_parse_option_ack(tftp_state_data_t *state,
>> const char
>> *ptr, int len)
>> +{
>> + const char *tmp = ptr;
>> + struct SessionHandle *data = state->conn->data;
>> +
>> + /* if OACK doesn't contain blksize option, the default (512)
>> must be used */
>> + state->blksize = TFTP_BLKSIZE_DEFAULT;
>> +
>> + while (tmp < ptr + len) {
>> + const char *option, *value;
>> +
>> + tmp = tftp_option_get(tmp, len + (ptr - tmp), &option, &value);
>
> The parenthesis here are confusing, IMHO.
>
>> + if(tmp == NULL) {
>> + failf(data, "%s\n", "Malformed ACK packet, rejecting");
>
> failf strings shouldn't end with \n. Also, using %s is redundant--
> just put
> the message in there.
>
>> + return CURLE_TFTP_ILLEGAL;
>> + }
>> +
>> + if(strncasecmp(option, TFTP_OPTION_BLKSIZE,
>> strlen(TFTP_OPTION_BLKSIZE)) =
>> = 0) {
>
> Use checkprefix here, as that's more portable and doesn't break in
> some
> locales. Same goes for all the strncasecmp calls.
>
>> + }
>> + else if(strncasecmp(option, TFTP_OPTION_TSIZE,
>> strlen(TFTP_OPTION_TSIZE))
>> == 0) {
>> + int tsize;
>> +
>> + tsize = strtol( value, NULL, 10 );
>
> This should probably be using a long (or curl_off_t) to support the
> (rather
> improbable but possible) case of downloading a > 2 GB file via TFTP.
>
>> +static int tftp_option_add(tftp_state_data_t *state, int csize,
>> char *buf,
>> char *option)
>
> option should be const char*.
>
>> +{
>> + if( ( strlen(option) + csize + 1 ) > state->blksize )
>> + return 0;
>> + strcpy(buf, option);
>> + return( strlen(option) + 1 );
>
> This could be simplified with Curl_strlcat.
>
>> +}
>> +
>> static CURLcode tftp_send_first(tftp_state_data_t *state,
>> tftp_event_t event)
>> {
>> int sbytes;
>> const char *mode = "octet";
>> char *filename;
>> + char buf[8];
>> struct SessionHandle *data = state->conn->data;
>> CURLcode res = CURLE_OK;
>>
>> @@ -318,7 +419,7 @@
>> /* If we are uploading, send an WRQ */
>> setpacketevent(&state->spacket, TFTP_EVENT_WRQ);
>> state->conn->data->req.upload_fromhere =
>> - (char *)&state->spacket.data[4];
>> + (char *)state->spacket.data+4;
>> if(data->set.infilesize != -1)
>> Curl_pgrsSetUploadSize(data, data->set.infilesize);
>> }
>> @@ -333,11 +434,30 @@
>> if(!filename)
>> return CURLE_OUT_OF_MEMORY;
>>
>> - snprintf((char *)&state->spacket.data[2],
>> - TFTP_BLOCKSIZE,
>> + snprintf((char *)state->spacket.data+2,
>> + state->blksize,
>> "%s%c%s%c", filename, '\0', mode, '\0');
>> sbytes = 4 + (int)strlen(filename) + (int)strlen(mode);
>> - sbytes = sendto(state->sockfd, (void *)&state->spacket,
>> +
>> + /* add tsize option */
>> + sbytes += tftp_option_add(state, sbytes,
>> + (char *)state->spacket.data+sbytes, TFTP_OPTION_TSIZE);
>
> It would be nice if there were some way to disable sending these
> options
> in case a server doesn't react well to their presence. It could
> easily be
> done by adding them only if the user explicitly specified a block
> size, but
> then you lose the potential benefit of an automatic tsize in all
> cases. If
> might be a sufficient trade-off, though, since most new TFTP-using
> apps are
> going to want to set a large blocksize anyway.
>
>> + **********************************************************/
>> +static CURLcode tftp_disconnect(struct connectdata *conn)
>> +{
>> + tftp_state_data_t *state = conn->data->state.proto.tftp;
>> +
>> + /* done, free dynamically allocated pkt buffers */
>> + if(state) {
>> + if(state->rpacket.data) {
>> + free(state->rpacket.data);
>> + }
>> + if(state->spacket.data) {
>> + free(state->spacket.data);
>
> You could just use Curl_safefree here.
>
>>>> Dan
> --
> http://www.MoveAnnouncer.com The web change of address
> service
> Let webmasters know that your web site has moved

Dan,

Thanks very much for your comments. Most code I've done up to this
point has been platform specific and I have also never written code
for libcurl. I'll go back and fix these the best I can and also use
libcurl internal functions for memory allocation and string operations.

I agree that it may be nice to disable inclusion of the options, but I
believe all TFTP servers which don't support options should just
ignore it and move on to sending data immediately. Maybe if we see any
issues we could add a setopt to disable option inclusion in the RRQ
and WRQ? I suspect we won't see any issues, but you never know what
kind of server you'll be interoperating with to out there.

After making the changes above and re-testing in my environment, I'll
add another patch to this thread. After posting I noticed a small bug
with handling of the OACK when performing a WRQ operation, so I'll fix
that too. Thanks again!

--
Chad Monroe