mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: DI PEDE Michele <michele.dipede@altran.com>
To: Antony Pavlov <antonynpavlov@gmail.com>
Cc: barebox <barebox@lists.infradead.org>,
	LACAMERA Daniele <daniele.lacamera@altran.com>
Subject: RE: picotcp barebox integration
Date: Sun, 1 Mar 2015 16:15:54 +0000	[thread overview]
Message-ID: <66FFD9910051464CB128685455FE423D2FB77B@XMB-DCFR-31.europe.corp.altran.com> (raw)
In-Reply-To: <20150301165425.ae74fe890992a1a47aac4294@gmail.com>

On Sun 1 Mar 2015 16:54:25,
Antony Pavlov <antonynpavlov@gmail.com>  wrote:
> On Sun, 1 Mar 2015 01:29:56 +0100
> 
> DI PEDE Michele <michele.dipede@altran.com> wrote:
> > On Sat 28 Feb 2015 21:01:01
> 
> ...
> 
> > Please take a look at the application driven API.
> > 
> > In order to integrate our stack you should use the application driven API.
> > It is documented in user manual in section 3.12 (TFTP). In order to
> > receive a file the sequence should be:
> > session = pico_tftp_app_setup(&server_address, short_be(PICO_TFTP_PORT),
> > PICO_PROTO_IPV4, &synchro);
> > ret = pico_tftp_app_start_rx(session, filename);
> > ret = pico_tftp_get_file_size(session, &file_size);
> > if (ret)
> > printf("Information about file size has not been received"\n);
> > 
> > while (file_is_not_complete) {
> > 
> >         len = pico_tftp_get(session, buf, PICO_TFTP_PAYLOAD_SIZE);
> >         ....
> > 
> > }
> > 
> > The example contained in test/test_tftp_app_client.c demonstrate how to
> > use
> > the interface for application driven behaviour.
> 
> Hmm. I have tried pico_tftp_get() and now I have several questions:
> 
> 1. pico_tftp_get() has 3rd argument 'int32_t len'.
> This 'len' argument is actually unused. Can we just drop it?
No, current payload size is fixed to 512 as stated in TFTP specification but we plan to add also RFC 2348 to our features. Code to handle it is almost there, I need just some time to clean it up and perform validation test. We will release it soon.

> 
> 2. here is pico_tftp_get() example fragment:
> 
>         len = pico_tftp_get(session, buf, PICO_TFTP_PAYLOAD_SIZE);
>         if (len < 0) {
>             fprintf(stderr, "Failure in pico_tftp_get\n");
>             close(fd);
>             countdown = 1;
>             continue;
>         }
>         ret = write(fd, buf, len);
> 
> pico_tftp_get() copies data to pointer buf. But the first four bytes are not
> raw file data but tftp header fields.
> E.g. I receiving GPL text via pico_tftp_get():
> 
> first tftp data block:
> 
> 00000000: 00 03 00 01 09 09 20 20 20 20 47 4e 55 20 47 45    ......    GNU
> GE 00000010: 4e 45 52 41 4c 20 50 55 42 4c 49 43 20 4c 49 43    NERAL
> PUBLIC LIC 00000020: 45 4e 53 45 0a 09 09 20 20 20 20 20 20 20 56 65   
> ENSE...       Ve ...
> 000001d0: 65 72 61 6c 20 50 75 62 6c 69 63 0a 4c 69 63 65    eral
> Public.Lice 000001e0: 6e 73 65 20 69 73 20 69 6e 74 65 6e 64 65 64 20   
> nse is intended 000001f0: 74 6f 20 67 75 61 72 61 6e 74 65 65              
>  to guarantee
> 
> second tftp data block:
> 
> 00000000: 00 03 00 02 65 65 64 6f 6d 20 74 6f 20 73 68 61    ....eedom to
> sha 00000010: 72 65 20 61 6e 64 20 63 68 61 6e 67 65 20 66 72    re and
> change fr 00000020: 65 65 0a 73 6f 66 74 77 61 72 65 2d 2d 74 6f 20   
> ee.software--to 00000030: 6d 61 6b 65 20 73 75 72 65 20 74 68 65 20 73 6f  
>  make sure the so 00000040: 66 74 77 61 72 65 20 69 73 20 66 72 65 65 20 66
>    ftware is free f
> 
> Here is a original GPL file dump, you can see that substring ' your fr'
> is missed in data received by pico_tftp_get():
> 
> 000001d0:  20 50 75 62 6c 69 63 0a  4c 69 63 65 6e 73 65 20   Public.License
> 000001e0:  69 73 20 69 6e 74 65 6e  64 65 64 20 74 6f 20 67  is intended to
> g 000001f0:  75 61 72 61 6e 74 65 65  20 79 6f 75 72 20 66 72  uarantee
> your fr <<<<<< 00000200:  65 65 64 6f 6d 20 74 6f  20 73 68 61 72 65 20 61 
> eedom to share a <<<<<< 00000210:  6e 64 20 63 68 61 6e 67  65 20 66 72 65
> 65 0a 73  nd change free.s 00000220:  6f 66 74 77 61 72 65 2d  2d 74 6f 20
> 6d 61 6b 65  oftware--to make
> 
> So data block written to file by
>     ret = write(fd, buf, len);
> is corrupted.
> 
> Could you please check this?
This is a BUG. Unfortunately pico_tftp_get returns the raw data but it is expected to returns only payload data. Thanks for signalling it. I've fixed the bug in the development branch. The commit is

commit 82fac1df278ab98cf0c46644de0fd608e341e5dd
Author: Michele Di Pede <michele.di.pede@tass.be>
Date:   Sun Mar 1 16:28:51 2015 +0100

    pico_tftp_get now return only payload data (issue #221)

> 
> 3. there is the find_session_by_socket() function. It uses special
> tftp_sessions list. Can we simply use priv field in 'struct pico_socket'
> for this purpose?
> 
> 
> 4. How I can determine "End Of File reached" situation after pico_tftp_get()
> call?
On receiver side you detect the End Of File reached when the size of the received packed less than the PACKET SIZE (it could be even 0). For this reason when you are the sender you have to send an extra packet with a len of 0 for payload (TFTP protocol rules).
Section 3.12.16 of the user manual show how to detect the EOF condition.
Currently this rule is explained only in the general description of TFTP (intro of 3.12) and in the example code. I'll make the description of TFTP a bit more verbose.

The lines that follow the snippet you showed above (extracted by test_tftp_app_client.c) are:

    if (ret < 0) {
        fprintf(stderr, "Error in write\n");
        pico_tftp_abort(session, TFTP_ERR_EXCEEDED, "File write error");
        close(fd);
        countdown = 1;
        continue;
    }
    printf("Written %" PRId32 " bytes to file (synchro=%d)\n", len, *synchro);

    if (len != PICO_TFTP_PAYLOAD_SIZE) {
        close(fd);
        printf("Transfer complete!\n");
        countdown = 1;
    }

First if block show how to signal an error to the remote side. Last if block how to recognize that the file transfer has been completed.

> 
> -- 
> Best regards,
>   Antony Pavlov

Best regards,
Michele Di Pede
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

  reply	other threads:[~2015-03-01 16:16 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-21 14:25 Antony Pavlov
2014-11-28 19:38 ` Daniele Lacamera
2014-12-19 17:28   ` Michele Di Pede
2015-01-28 13:35     ` DI PEDE Michele
2015-02-28 17:01       ` Antony Pavlov
2015-03-01  0:29         ` DI PEDE Michele
2015-03-01 12:54           ` Antony Pavlov
2015-03-01 16:15             ` DI PEDE Michele [this message]
2015-03-31 12:50               ` Antony Pavlov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=66FFD9910051464CB128685455FE423D2FB77B@XMB-DCFR-31.europe.corp.altran.com \
    --to=michele.dipede@altran.com \
    --cc=antonynpavlov@gmail.com \
    --cc=barebox@lists.infradead.org \
    --cc=daniele.lacamera@altran.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox