[Qemu-devel] [PATCH] [tftp] Handle TFTP ERROR from client

[Qemu-devel] [PATCH] [tftp] Handle TFTP ERROR from client

[Thomas Horsten]

If a PXE client only wants to find out the size of a file, it will
open the file and then abort the transfer by sending a TFTP ERROR packet.

The ERROR packet should cause qemu to terminate the session. If not,
the sessions will soon run out and cause timeouts in the client.

Also, if a TFTP session already exists with same IP/UDP port, it
should be terminated when a new RRQ is received, instead of creating a
duplicate (which will never be used).

A patch for gPXE to send the ERROR packet is also being submitted to
gPXE. Together they resolve slowness/hanging when booting pxegrub from
qemu's internal TFTP server. The patch from Milan Plzik to return
after sending OACK is also required for a complete fix.

Signed-off-by: Thomas Horsten <thomas@horsten.com>
Signed-off-by: Milan Plzik <milan.plzik@gmail.com>
---
 slirp/tftp.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index db869fc..96c0e0c 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -264,6 +264,12 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
   size_t prefix_len;
   char *req_fname;
 
+  /* check if a session already exists and if so terminate it */
+  s = tftp_session_find(slirp, tp);
+  if (s >= 0) {
+    tftp_session_terminate(&slirp->tftp_sessions[s]);
+  }
+
   s = tftp_session_allocate(slirp, tp);
 
   if (s < 0) {
@@ -386,6 +392,19 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
   }
 }
 
+static void tftp_handle_error(Slirp *slirp, struct tftp_t *tp, int pktlen)
+{
+  int s;
+
+  s = tftp_session_find(slirp, tp);
+
+  if (s < 0) {
+    return;
+  }
+
+  tftp_session_terminate(&slirp->tftp_sessions[s]);
+}
+
 void tftp_input(struct mbuf *m)
 {
   struct tftp_t *tp = (struct tftp_t *)m->m_data;
@@ -398,5 +417,9 @@ void tftp_input(struct mbuf *m)
   case TFTP_ACK:
     tftp_handle_ack(m->slirp, tp, m->m_len);
     break;
+
+  case TFTP_ERROR:
+    tftp_handle_error(m->slirp, tp, m->m_len);
+    break;
   }
 }
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH] [tftp] Handle TFTP ERROR from client

[Anthony Liguori]

On 01/07/2010 11:01 AM, Thomas Horsten wrote:
> If a PXE client only wants to find out the size of a file, it will
> open the file and then abort the transfer by sending a TFTP ERROR packet.
>
> The ERROR packet should cause qemu to terminate the session. If not,
> the sessions will soon run out and cause timeouts in the client.
>
> Also, if a TFTP session already exists with same IP/UDP port, it
> should be terminated when a new RRQ is received, instead of creating a
> duplicate (which will never be used).
>
> A patch for gPXE to send the ERROR packet is also being submitted to
> gPXE. Together they resolve slowness/hanging when booting pxegrub from
> qemu's internal TFTP server. The patch from Milan Plzik to return
> after sending OACK is also required for a complete fix.
>
> Signed-off-by: Thomas Horsten<thomas@horsten.com>
> Signed-off-by: Milan Plzik<milan.plzik@gmail.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   slirp/tftp.c |   23 +++++++++++++++++++++++
>   1 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index db869fc..96c0e0c 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -264,6 +264,12 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
>     size_t prefix_len;
>     char *req_fname;
>
> +  /* check if a session already exists and if so terminate it */
> +  s = tftp_session_find(slirp, tp);
> +  if (s>= 0) {
> +    tftp_session_terminate(&slirp->tftp_sessions[s]);
> +  }
> +
>     s = tftp_session_allocate(slirp, tp);
>
>     if (s<  0) {
> @@ -386,6 +392,19 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
>     }
>   }
>
> +static void tftp_handle_error(Slirp *slirp, struct tftp_t *tp, int pktlen)
> +{
> +  int s;
> +
> +  s = tftp_session_find(slirp, tp);
> +
> +  if (s<  0) {
> +    return;
> +  }
> +
> +  tftp_session_terminate(&slirp->tftp_sessions[s]);
> +}
> +
>   void tftp_input(struct mbuf *m)
>   {
>     struct tftp_t *tp = (struct tftp_t *)m->m_data;
> @@ -398,5 +417,9 @@ void tftp_input(struct mbuf *m)
>     case TFTP_ACK:
>       tftp_handle_ack(m->slirp, tp, m->m_len);
>       break;
> +
> +  case TFTP_ERROR:
> +    tftp_handle_error(m->slirp, tp, m->m_len);
> +    break;
>     }
>   }
>    

Re: [Qemu-devel] [PATCH] [tftp] Handle TFTP ERROR from client

[Thomas Horsten]

2010/1/11 Anthony Liguori <anthony@codemonkey.ws>:

> On 01/07/2010 11:01 AM, Thomas Horsten wrote:

>> ...The patch from Milan Plzik to return
>> after sending OACK is also required for a complete fix.

> Applied.  Thanks.

Thanks. It appears that the patch Milan submitted was missed (probably
because it was in a response to a thread), could you please apply that
one as well?

The subject was "Qemu's internal TFTP server breaks lock-step-iness of
TFTP", and it's archived here:
http://lists.gnu.org/archive/html/qemu-devel/2010-01/msg00449.html

Thomas

Re: [Qemu-devel] [PATCH] [tftp] Handle TFTP ERROR from client

[Anthony Liguori]

On 01/12/2010 04:38 PM, Thomas Horsten wrote:
> 2010/1/11 Anthony Liguori<anthony@codemonkey.ws>:
>
>    
>> On 01/07/2010 11:01 AM, Thomas Horsten wrote:
>>      
>    
>>> ...The patch from Milan Plzik to return
>>> after sending OACK is also required for a complete fix.
>>>        
>    
>> Applied.  Thanks.
>>      
> Thanks. It appears that the patch Milan submitted was missed (probably
> because it was in a response to a thread), could you please apply that
> one as well?
>
> The subject was "Qemu's internal TFTP server breaks lock-step-iness of
> TFTP", and it's archived here:
> http://lists.gnu.org/archive/html/qemu-devel/2010-01/msg00449.html
>    

Yeah, I'll queue it.  It lacked a [PATCH] in the subject.

Regards,

Anthony LIguori

> Thomas
>