Bugs item #2723236, was opened at 2009-03-31 14:20
Message generated for change (Settings changed) made by bagder
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=2723236&group_id=976

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: TFTP
Group: wrong behaviour
>Status: Closed
Resolution: Fixed
Priority: 5
Private: No
Submitted By: Vijay G (vijayg23333)
Assigned to: Daniel Stenberg (bagder)
Summary: TFTP file transfer issue in Curl 7.19.4

Initial Comment:
In Curl lib 7.19.4, tftp_tx() is not handling the case for TFTP_EVENT_OACK. This case should be added in tftp_tx(). In tftp_rx(), this case is already handled.
In RFC 2348,it is given as "If the server is willing to accept the blocksize option, it sends an Option Acknowledgment (OACK) to the client."
I was not able to upload files, as we are not handling this scenario.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-05-07 10:31

Message:
Thanks, this was committed to CVS now. It'd be great if you could verify
that it works for you.

Also, if you tell me your real name I'll credit you properly in changelogs
etc!

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-05-06 08:27

Message:
I think the changes in your patch should be enough.Thanks.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-05-04 15:15

Message:
Ok, seeing that your patch introduces a lot of code duplication. How about
simply doing the change as I did in my just uploaded tftp-oack.patch ?

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-04-30 13:21

Message:
I can try changing the size where?

I asked you about changing the tftp server to work with this.

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-04-30 12:51

Message:
You can try changing the TFTP blocksize from 512 to 2048 or something. If
upload is working, then thats great.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-04-30 09:44

Message:
To me that's just more reasons to add support for this in the test suite so
that we can be sure our stuff works. We have a test TFTP server there and a
first step would thus be to make sure it supports RFC2348, I think. Do you
think you could have a look at it? It doesn't even have to be a
full-fledged implementation but it would be good with at least some kind of
awareness so that we can use that to make sure curl behaves at least mostly
right.

And I do believe someone has used such a server as the guy who implemented
this feature probably didn't do it without using it at all.

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-04-30 08:23

Message:
My doubt is that the tftp file upload/download has not been tested on a
server which supports RFC2348.

On TFTP servers which do not support RFC2348, no issue will be seen since
the server will not respond with an OACK message. But TFTP servers which
support RFC 2348 will send an OACK to the client. If this case is not
handled in tftp_tx(), file upload will fail. When the same OACK is handled
in tftp_rx(), it should be handled in tftp_tx() as well.

RFC 2348 says "If the server is willing to accept the blocksize option, it
sends an
   Option Acknowledgment (OACK) to the client."

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-04-29 13:36

Message:
So there's no chance you can help us make the test suite test for this? I'm
still reluctant to add this without understanding it further.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-04-17 13:26

Message:
Test case 285 and 286 already do TFTP uploads and they've worked for a long
time. So no, "just uploading" is not a proper test case. It clearly
requires something more special than so...

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-04-17 13:20

Message:
Without this fix I was not able to upload any files. That is the only test
case I can think of.

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-04-13 20:17

Message:
Any chance you can write up a test case for the test suite that shows the
problem and thus how this fix cures it?

I don't see any TFTP expert commenting on this patch and I personally know
little about these details and I haven't had the time to read up so it
would help a lot.

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-04-10 09:30

Message:
Daniel,

Can I know when these changes will be merged to the tftp.c file

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-04-01 11:43

Message:
This is the patch created using ""diff -u"

--- tftp_1.80.c 2009-03-25 09:34:33.007209600 +0530
+++ tftp.c 2009-04-01 15:05:48.895218000 +0530
@@ -723,6 +723,37 @@
     Curl_pgrsSetUploadCounter(data, k->writebytecount);
     break;
 
+case TFTP_EVENT_OACK:
+ /* This is the expected packet. Reset the counters and send the next
+ block */
+ state->block++;
+ state->retries = 0;
+ setpacketevent(&state->spacket, TFTP_EVENT_DATA);
+ setpacketblock(&state->spacket, state->block);
+ if(state->block > 1 && state->sbytes < state->blksize) {
+ state->state = TFTP_STATE_FIN;
+ return CURLE_OK;
+ }
+ res = Curl_fillreadbuffer(state->conn, state->blksize,
+ (int *)&state->sbytes);
+ if(res)
+ return res;
+ sbytes = sendto(state->sockfd, (void *)state->spacket.data,
+ 4+state->sbytes, SEND_4TH_ARG,
+ (struct sockaddr *)&state->remote_addr,
+ state->remote_addrlen);
+ /* Check all sbytes were sent */
+ if(sbytes<0) {
+ failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
+ return CURLE_SEND_ERROR;
+ }
+ /* Update the progress meter */
+ k->writebytecount += state->sbytes;
+ Curl_pgrsSetUploadCounter(data, k->writebytecount);
+
+break;
+
+
   case TFTP_EVENT_TIMEOUT:
     /* Increment the retry counter and log the timeout */
     state->retries++;

----------------------------------------------------------------------

Comment By: Daniel Stenberg (bagder)
Date: 2009-03-31 14:24

Message:
Please make the patch with 'diff -u' and post that here instead of a full
code snippet.

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-03-31 14:24

Message:
Ignore the line #ifndef CURL_DISABLE_PROGRESS.

I have added the case statement for TFTP_EVENT_OACK to fix the issue.

----------------------------------------------------------------------

Comment By: Vijay G (vijayg23333)
Date: 2009-03-31 14:22

Message:
The patch for this issue is

/**********************************************************
 *
 * tftp_tx
 *
 * Event handler for the TX state
 *
 **********************************************************/
static CURLcode tftp_tx(tftp_state_data_t *state, tftp_event_t event)
{
  struct SessionHandle *data = state->conn->data;
  int sbytes;
  int rblock;
  CURLcode res = CURLE_OK;
  struct SingleRequest *k = &data->req;

  switch(event) {

  case TFTP_EVENT_ACK:
    /* Ack the packet */
    rblock = getrpacketblock(&state->rpacket);

    if(rblock != state->block) {
      /* This isn't the expected block. Log it and up the retry counter
*/
      infof(data, "Received ACK for block %d, expecting %d\n",
            rblock, state->block);
      state->retries++;
      /* Bail out if over the maximum */
      if(state->retries>state->retry_max) {
        failf(data, "tftp_tx: giving up waiting for block %d ack",
              state->block);
        res = CURLE_SEND_ERROR;
      }
      else {
        /* Re-send the data packet */
        sbytes = sendto(state->sockfd, (void *)&state->spacket,
                        4+state->sbytes, SEND_4TH_ARG,
                        (struct sockaddr *)&state->remote_addr,
                        state->remote_addrlen);
        /* Check all sbytes were sent */
        if(sbytes<0) {
          failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
          res = CURLE_SEND_ERROR;
        }
      }
      return res;
    }
    /* This is the expected packet. Reset the counters and send the next
       block */
    state->block++;
    state->retries = 0;
    setpacketevent(&state->spacket, TFTP_EVENT_DATA);
    setpacketblock(&state->spacket, state->block);
    if(state->block > 1 && state->sbytes < state->blksize) {
      state->state = TFTP_STATE_FIN;
      return CURLE_OK;
    }
    res = Curl_fillreadbuffer(state->conn, state->blksize,
                              (int *)&state->sbytes);
    if(res)
      return res;
    sbytes = sendto(state->sockfd, (void *)state->spacket.data,
                    4+state->sbytes, SEND_4TH_ARG,
                    (struct sockaddr *)&state->remote_addr,
                    state->remote_addrlen);
    /* Check all sbytes were sent */
    if(sbytes<0) {
      failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
      return CURLE_SEND_ERROR;
    }
    /* Update the progress meter */
    k->writebytecount += state->sbytes;
    Curl_pgrsSetUploadCounter(data, k->writebytecount);
    break;
case TFTP_EVENT_OACK:
 /* This is the expected packet. Reset the counters and send the next
       block */
    state->block++;
    state->retries = 0;
    setpacketevent(&state->spacket, TFTP_EVENT_DATA);
    setpacketblock(&state->spacket, state->block);
    if(state->block > 1 && state->sbytes < state->blksize) {
      state->state = TFTP_STATE_FIN;
      return CURLE_OK;
    }
    res = Curl_fillreadbuffer(state->conn, state->blksize,
                              (int *)&state->sbytes);
    if(res)
      return res;
    sbytes = sendto(state->sockfd, (void *)state->spacket.data,
                    4+state->sbytes, SEND_4TH_ARG,
                    (struct sockaddr *)&state->remote_addr,
                    state->remote_addrlen);
    /* Check all sbytes were sent */
    if(sbytes<0) {
      failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
      return CURLE_SEND_ERROR;
    }
    /* Update the progress meter */
    k->writebytecount += state->sbytes;
#ifndef CURL_DISABLE_PROGRESS
    Curl_pgrsSetUploadCounter(data, k->writebytecount);
#endif
   
break;

  case TFTP_EVENT_TIMEOUT:
    /* Increment the retry counter and log the timeout */
    state->retries++;
    infof(data, "Timeout waiting for block %d ACK. "
          " Retries = %d\n", state->retries);
    /* Decide if we've had enough */
    if(state->retries > state->retry_max) {
      state->error = TFTP_ERR_TIMEOUT;
      state->state = TFTP_STATE_FIN;
    }
    else {
      /* Re-send the data packet */
      sbytes = sendto(state->sockfd, (void *)state->spacket.data,
                      4+state->sbytes, SEND_4TH_ARG,
                      (struct sockaddr *)&state->remote_addr,
                      state->remote_addrlen);
      /* Check all sbytes were sent */
      if(sbytes<0) {
        failf(data, "%s", Curl_strerror(state->conn, SOCKERRNO));
        return CURLE_SEND_ERROR;
      }
      /* since this was a re-send, we remain at the still byte position */
      Curl_pgrsSetUploadCounter(data, k->writebytecount);
    }
    break;

  case TFTP_EVENT_ERROR:
    state->state = TFTP_STATE_FIN;
    break;

  default:
    failf(data, "%s", "tftp_tx: internal error");
    break;
  }

  return res;
}

----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100976&aid=2723236&group_id=976
Received on 2009-05-10