[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

The following changes since commit b0bcc86d2a87456f5a276f941dc775b265b309cf:

  Update version for v2.8.0-rc0 release (2016-11-15 20:55:12 +0000)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to 43fccf87c92a6a88a7294597b719f17fd1b41d3d:

  tftp: fake support for netascii protocol (2016-11-18 18:49:02 +0100)

----------------------------------------------------------------
slirp updates

----------------------------------------------------------------
Vincent Bernat (1):
      tftp: fake support for netascii protocol

 slirp/tftp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

From: Vincent Bernat <vincent@bernat.im>

Some network equipments are requesting a file using the netascii
protocol and this is not configurable. Currently, qemu's tftpd only
supports the octet protocol. This commit makes it accept the netascii
protocol as well but do not perform the requested transformation (LF ->
CR,LF) as it would be far more complex. The current implementation is
good enough. A user has always the choice to preencode the served file
correctly.

Signed-off-by: Vincent Bernat <vincent@bernat.im>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 slirp/tftp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c185906..ab1c05d 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -326,13 +326,15 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
     return;
   }
 
-  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      k += 9;
+  } else {
       tftp_send_error(spt, 4, "Unsupported transfer mode", tp);
       return;
   }
 
-  k += 6; /* skipping octet */
-
   /* do sanity checks on the filename */
   if (!strncmp(req_fname, "../", 3) ||
       req_fname[strlen(req_fname) - 1] == '/' ||
-- 
2.10.2

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PULL] tftp: fake support for netascii protocol
Type: series
Message-id: 20161118175128.17192-2-samuel.thibault@ens-lyon.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20161118175128.17192-2-samuel.thibault@ens-lyon.org -> patchew/20161118175128.17192-2-samuel.thibault@ens-lyon.org
Switched to a new branch 'test'
01931bf tftp: fake support for netascii protocol

=== OUTPUT BEGIN ===
Checking PATCH 1/1: tftp: fake support for netascii protocol...
ERROR: suspect code indent for conditional statements (2, 6)
#27: FILE: slirp/tftp.c:329:
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;

ERROR: suspect code indent for conditional statements (2, 6)
#29: FILE: slirp/tftp.c:331:
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      k += 9;

total: 2 errors, 0 warnings, 18 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Thomas Huth]

On 18.11.2016 18:51, Samuel Thibault wrote:
> From: Vincent Bernat <vincent@bernat.im>
> 
> Some network equipments are requesting a file using the netascii
> protocol and this is not configurable. Currently, qemu's tftpd only
> supports the octet protocol. This commit makes it accept the netascii
> protocol as well but do not perform the requested transformation (LF ->
> CR,LF) as it would be far more complex.

That sounds somewhat wrong to me. QEMU now seems to support a transfer
mode that is not really implemented. I think you should at least issue a
qemu_log_mask(LOG_UNIMP, "...") call in that case.

 Thomas


PS: As far as I know, this month, PULL requests should be CC:ed to
Stefan, not Peter, since Peter is away.

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Vincent Bernat]

 ❦ 19 novembre 2016 08:32 +0100, Thomas Huth <thuth@redhat.com> :

>> Some network equipments are requesting a file using the netascii
>> protocol and this is not configurable. Currently, qemu's tftpd only
>> supports the octet protocol. This commit makes it accept the netascii
>> protocol as well but do not perform the requested transformation (LF ->
>> CR,LF) as it would be far more complex.
>
> That sounds somewhat wrong to me. QEMU now seems to support a transfer
> mode that is not really implemented.

On the other hand, the current implementation may not RFC compliant as
the three modes (netascii, octet, mail) are not supported (but the RFC
predates the usage of MAY/SHOULD/MUST, so it's unclear for me).

I have tried to add proper support, but this is more invasive as we
cannot just seek in the file to get each block. However, this is
something that I can do if compliance is important for QEMU.

> I think you should at least issue a qemu_log_mask(LOG_UNIMP, "...")
> call in that case.

I can do that if needed.
-- 
10.0 times 0.1 is hardly ever 1.0.
            - The Elements of Programming Style (Kernighan & Plauger)

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

Vincent Bernat, on Sat 19 Nov 2016 09:03:08 +0100, wrote:
>  ❦ 19 novembre 2016 08:32 +0100, Thomas Huth <thuth@redhat.com> :
> 
> >> Some network equipments are requesting a file using the netascii
> >> protocol and this is not configurable. Currently, qemu's tftpd only
> >> supports the octet protocol. This commit makes it accept the netascii
> >> protocol as well but do not perform the requested transformation (LF ->
> >> CR,LF) as it would be far more complex.
> >
> > That sounds somewhat wrong to me. QEMU now seems to support a transfer
> > mode that is not really implemented.
> 
> On the other hand, the current implementation may not RFC compliant as
> the three modes (netascii, octet, mail) are not supported (but the RFC
> predates the usage of MAY/SHOULD/MUST, so it's unclear for me).
> 
> I have tried to add proper support, but this is more invasive as we
> cannot just seek in the file to get each block. However, this is
> something that I can do if compliance is important for QEMU.
> 
> > I think you should at least issue a qemu_log_mask(LOG_UNIMP, "...")
> > call in that case.
> 
> I can do that if needed.

That'd be better indeed. Otherwise people might wonder why things are
not working. Warning that they have to do the LF -> CR,LF conversion by
hand is important here.

Samuel

[Qemu-devel] [v2] tftp: fake support for netascii protocol

[Vincent Bernat]

From: Vincent Bernat <vincent@bernat.im>

Some network equipments are requesting a file using the netascii
protocol and this is not configurable. Currently, qemu's tftpd only
supports the octet protocol. This commit makes it accept the netascii
protocol as well but do not perform the requested transformation (LF ->
CR,LF) as it would be far more complex. The current implementation is
good enough. A user has always the choice to preencode the served file
correctly.

Signed-off-by: Vincent Bernat <vincent@bernat.im>
---
 slirp/tftp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c1859066ccb2..6907d5b92074 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -26,6 +26,7 @@
 #include "slirp.h"
 #include "qemu-common.h"
 #include "qemu/cutils.h"
+#include "qemu/log.h"
 
 static inline int tftp_session_in_use(struct tftp_session *spt)
 {
@@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
     return;
   }
 
-  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
+                    "no CR-LF conversion\n");
+      k += 9;
+  } else {
       tftp_send_error(spt, 4, "Unsupported transfer mode", tp);
       return;
   }
 
-  k += 6; /* skipping octet */
-
   /* do sanity checks on the filename */
   if (!strncmp(req_fname, "../", 3) ||
       req_fname[strlen(req_fname) - 1] == '/' ||
-- 
2.10.2

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [v2] tftp: fake support for netascii protocol
Message-id: 20161120084136.721-1-Vincent.Bernat@exoscale.ch
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20161119185318.10564-1-sw@weilnetz.de -> patchew/20161119185318.10564-1-sw@weilnetz.de
 * [new tag]         patchew/20161120084136.721-1-Vincent.Bernat@exoscale.ch -> patchew/20161120084136.721-1-Vincent.Bernat@exoscale.ch
Switched to a new branch 'test'
f8f1b3e tftp: fake support for netascii protocol

=== OUTPUT BEGIN ===
Checking PATCH 1/1: tftp: fake support for netascii protocol...
ERROR: suspect code indent for conditional statements (2, 6)
#34: FILE: slirp/tftp.c:330:
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;

ERROR: suspect code indent for conditional statements (2, 6)
#36: FILE: slirp/tftp.c:332:
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "

total: 2 errors, 0 warnings, 27 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [v2] tftp: fake support for netascii protocol
Type: series
Message-id: 20161120084136.721-1-Vincent.Bernat@exoscale.ch

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20161120084136.721-1-Vincent.Bernat@exoscale.ch -> patchew/20161120084136.721-1-Vincent.Bernat@exoscale.ch
Switched to a new branch 'test'
f8f1b3e tftp: fake support for netascii protocol

=== OUTPUT BEGIN ===
Checking PATCH 1/1: tftp: fake support for netascii protocol...
ERROR: suspect code indent for conditional statements (2, 6)
#34: FILE: slirp/tftp.c:330:
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;

ERROR: suspect code indent for conditional statements (2, 6)
#36: FILE: slirp/tftp.c:332:
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "

total: 2 errors, 0 warnings, 27 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Thomas Huth]

On 20.11.2016 09:41, Vincent Bernat wrote:
> From: Vincent Bernat <vincent@bernat.im>
> 
> Some network equipments are requesting a file using the netascii
> protocol and this is not configurable. Currently, qemu's tftpd only
> supports the octet protocol. This commit makes it accept the netascii
> protocol as well but do not perform the requested transformation (LF ->
> CR,LF) as it would be far more complex. The current implementation is
> good enough. A user has always the choice to preencode the served file
> correctly.
> 
> Signed-off-by: Vincent Bernat <vincent@bernat.im>
> ---
>  slirp/tftp.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index c1859066ccb2..6907d5b92074 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -26,6 +26,7 @@
>  #include "slirp.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
> +#include "qemu/log.h"
>  
>  static inline int tftp_session_in_use(struct tftp_session *spt)
>  {
> @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
>      return;
>    }
>  
> -  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
> +  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
> +      k += 6;
> +  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
> +      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
> +                    "no CR-LF conversion\n");
> +      k += 9;
> +  } else {
>        tftp_send_error(spt, 4, "Unsupported transfer mode", tp);
>        return;
>    }
>  
> -  k += 6; /* skipping octet */
> -
>    /* do sanity checks on the filename */
>    if (!strncmp(req_fname, "../", 3) ||
>        req_fname[strlen(req_fname) - 1] == '/' ||
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1853 bytes --]

On Sun, Nov 20, 2016 at 09:41:36AM +0100, Vincent Bernat wrote:
> From: Vincent Bernat <vincent@bernat.im>
> 
> Some network equipments are requesting a file using the netascii
> protocol and this is not configurable. Currently, qemu's tftpd only
> supports the octet protocol. This commit makes it accept the netascii
> protocol as well but do not perform the requested transformation (LF ->
> CR,LF) as it would be far more complex. The current implementation is
> good enough. A user has always the choice to preencode the served file
> correctly.
> 
> Signed-off-by: Vincent Bernat <vincent@bernat.im>
> ---
>  slirp/tftp.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index c1859066ccb2..6907d5b92074 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -26,6 +26,7 @@
>  #include "slirp.h"
>  #include "qemu-common.h"
>  #include "qemu/cutils.h"
> +#include "qemu/log.h"
>  
>  static inline int tftp_session_in_use(struct tftp_session *spt)
>  {
> @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
>      return;
>    }
>  
> -  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
> +  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
> +      k += 6;
> +  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
> +      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
> +                    "no CR-LF conversion\n");
> +      k += 9;
> +  } else {

This is an RFC violation.  I don't think it's suitable for upstream QEMU.

The commit description says it would be "far more complex" to implement
netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so hard?

CCing H. Peter Anvin who has TFTP expertise in case I'm missing
something.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[hpa]

On November 21, 2016 6:46:16 AM PST, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>On Sun, Nov 20, 2016 at 09:41:36AM +0100, Vincent Bernat wrote:
>> From: Vincent Bernat <vincent@bernat.im>
>> 
>> Some network equipments are requesting a file using the netascii
>> protocol and this is not configurable. Currently, qemu's tftpd only
>> supports the octet protocol. This commit makes it accept the netascii
>> protocol as well but do not perform the requested transformation (LF
>->
>> CR,LF) as it would be far more complex. The current implementation is
>> good enough. A user has always the choice to preencode the served
>file
>> correctly.
>> 
>> Signed-off-by: Vincent Bernat <vincent@bernat.im>
>> ---
>>  slirp/tftp.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>> 
>> diff --git a/slirp/tftp.c b/slirp/tftp.c
>> index c1859066ccb2..6907d5b92074 100644
>> --- a/slirp/tftp.c
>> +++ b/slirp/tftp.c
>> @@ -26,6 +26,7 @@
>>  #include "slirp.h"
>>  #include "qemu-common.h"
>>  #include "qemu/cutils.h"
>> +#include "qemu/log.h"
>>  
>>  static inline int tftp_session_in_use(struct tftp_session *spt)
>>  {
>> @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp,
>struct sockaddr_storage *srcsas,
>>      return;
>>    }
>>  
>> -  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
>> +  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
>> +      k += 6;
>> +  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
>> +      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not
>implemented, "
>> +                    "no CR-LF conversion\n");
>> +      k += 9;
>> +  } else {
>
>This is an RFC violation.  I don't think it's suitable for upstream
>QEMU.
>
>The commit description says it would be "far more complex" to implement
>netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so hard?
>
>CCing H. Peter Anvin who has TFTP expertise in case I'm missing
>something.
>
>Stefan

It should be trivial to do.  The one caveat is that the tsize extension needs to be disabled for a netascii read.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Samuel Thibault]

Stefan Hajnoczi, on Mon 21 Nov 2016 14:46:16 +0000, wrote:
> On Sun, Nov 20, 2016 at 09:41:36AM +0100, Vincent Bernat wrote:
> > From: Vincent Bernat <vincent@bernat.im>
> > 
> > Some network equipments are requesting a file using the netascii
> > protocol and this is not configurable. Currently, qemu's tftpd only
> > supports the octet protocol. This commit makes it accept the netascii
> > protocol as well but do not perform the requested transformation (LF ->
> > CR,LF) as it would be far more complex. The current implementation is
> > good enough. A user has always the choice to preencode the served file
> > correctly.
> > 
> > Signed-off-by: Vincent Bernat <vincent@bernat.im>
> > ---
> >  slirp/tftp.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/slirp/tftp.c b/slirp/tftp.c
> > index c1859066ccb2..6907d5b92074 100644
> > --- a/slirp/tftp.c
> > +++ b/slirp/tftp.c
> > @@ -26,6 +26,7 @@
> >  #include "slirp.h"
> >  #include "qemu-common.h"
> >  #include "qemu/cutils.h"
> > +#include "qemu/log.h"
> >  
> >  static inline int tftp_session_in_use(struct tftp_session *spt)
> >  {
> > @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
> >      return;
> >    }
> >  
> > -  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
> > +  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
> > +      k += 6;
> > +  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
> > +      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
> > +                    "no CR-LF conversion\n");
> > +      k += 9;
> > +  } else {
> 
> This is an RFC violation.  I don't think it's suitable for upstream QEMU.
> 
> The commit description says it would be "far more complex" to implement
> netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so hard?

I guess the question is that while the patch above could be accepted for
the upcoming 2.8 (I don't see it breaking existing systems), a patch
that would implement the transformation would be a lot more involved,
and really not suitable for 2.8.

Samuel

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[hpa]

On November 21, 2016 7:05:41 AM PST, Samuel Thibault <samuel.thibault@gnu.org> wrote:
>Stefan Hajnoczi, on Mon 21 Nov 2016 14:46:16 +0000, wrote:
>> On Sun, Nov 20, 2016 at 09:41:36AM +0100, Vincent Bernat wrote:
>> > From: Vincent Bernat <vincent@bernat.im>
>> > 
>> > Some network equipments are requesting a file using the netascii
>> > protocol and this is not configurable. Currently, qemu's tftpd only
>> > supports the octet protocol. This commit makes it accept the
>netascii
>> > protocol as well but do not perform the requested transformation
>(LF ->
>> > CR,LF) as it would be far more complex. The current implementation
>is
>> > good enough. A user has always the choice to preencode the served
>file
>> > correctly.
>> > 
>> > Signed-off-by: Vincent Bernat <vincent@bernat.im>
>> > ---
>> >  slirp/tftp.c | 11 ++++++++---
>> >  1 file changed, 8 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/slirp/tftp.c b/slirp/tftp.c
>> > index c1859066ccb2..6907d5b92074 100644
>> > --- a/slirp/tftp.c
>> > +++ b/slirp/tftp.c
>> > @@ -26,6 +26,7 @@
>> >  #include "slirp.h"
>> >  #include "qemu-common.h"
>> >  #include "qemu/cutils.h"
>> > +#include "qemu/log.h"
>> >  
>> >  static inline int tftp_session_in_use(struct tftp_session *spt)
>> >  {
>> > @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp,
>struct sockaddr_storage *srcsas,
>> >      return;
>> >    }
>> >  
>> > -  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
>> > +  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
>> > +      k += 6;
>> > +  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
>> > +      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not
>implemented, "
>> > +                    "no CR-LF conversion\n");
>> > +      k += 9;
>> > +  } else {
>> 
>> This is an RFC violation.  I don't think it's suitable for upstream
>QEMU.
>> 
>> The commit description says it would be "far more complex" to
>implement
>> netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so
>hard?
>
>I guess the question is that while the patch above could be accepted
>for
>the upcoming 2.8 (I don't see it breaking existing systems), a patch
>that would implement the transformation would be a lot more involved,
>and really not suitable for 2.8.
>
>Samuel

A client that asks for netascii and falls back to octet if the requests fail could break.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]

On Mon, Nov 21, 2016 at 04:05:41PM +0100, Samuel Thibault wrote:
> Stefan Hajnoczi, on Mon 21 Nov 2016 14:46:16 +0000, wrote:
> > On Sun, Nov 20, 2016 at 09:41:36AM +0100, Vincent Bernat wrote:
> > > From: Vincent Bernat <vincent@bernat.im>
> > > 
> > > Some network equipments are requesting a file using the netascii
> > > protocol and this is not configurable. Currently, qemu's tftpd only
> > > supports the octet protocol. This commit makes it accept the netascii
> > > protocol as well but do not perform the requested transformation (LF ->
> > > CR,LF) as it would be far more complex. The current implementation is
> > > good enough. A user has always the choice to preencode the served file
> > > correctly.
> > > 
> > > Signed-off-by: Vincent Bernat <vincent@bernat.im>
> > > ---
> > >  slirp/tftp.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/slirp/tftp.c b/slirp/tftp.c
> > > index c1859066ccb2..6907d5b92074 100644
> > > --- a/slirp/tftp.c
> > > +++ b/slirp/tftp.c
> > > @@ -26,6 +26,7 @@
> > >  #include "slirp.h"
> > >  #include "qemu-common.h"
> > >  #include "qemu/cutils.h"
> > > +#include "qemu/log.h"
> > >  
> > >  static inline int tftp_session_in_use(struct tftp_session *spt)
> > >  {
> > > @@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
> > >      return;
> > >    }
> > >  
> > > -  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
> > > +  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
> > > +      k += 6;
> > > +  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
> > > +      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
> > > +                    "no CR-LF conversion\n");
> > > +      k += 9;
> > > +  } else {
> > 
> > This is an RFC violation.  I don't think it's suitable for upstream QEMU.
> > 
> > The commit description says it would be "far more complex" to implement
> > netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so hard?
> 
> I guess the question is that while the patch above could be accepted for
> the upcoming 2.8 (I don't see it breaking existing systems), a patch
> that would implement the transformation would be a lot more involved,
> and really not suitable for 2.8.

This pull request cannot be accepted for QEMU 2.8 because it is not a
bug fix.  QEMU is in hard feature freeze (until mid-December):
http://qemu-project.org/Planning/HardFeatureFreeze
http://qemu-project.org/Planning/2.8

If you decide to implement netascii in the future, please update the
documentation in qemu-options.hx:

  The TFTP client on the guest must be configured in binary mode (use
  the command @code{bin} of the Unix TFTP client).

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Vincent Bernat]

 ❦ 22 novembre 2016 10:49 GMT, Stefan Hajnoczi <stefanha@gmail.com> :

>> I guess the question is that while the patch above could be accepted for
>> the upcoming 2.8 (I don't see it breaking existing systems), a patch
>> that would implement the transformation would be a lot more involved,
>> and really not suitable for 2.8.
>
> This pull request cannot be accepted for QEMU 2.8 because it is not a
> bug fix.  QEMU is in hard feature freeze (until mid-December):
> http://qemu-project.org/Planning/HardFeatureFreeze
> http://qemu-project.org/Planning/2.8

So, I'll implement netascii in the coming weeks and update the proposed
patch.
-- 
Don't use conditional branches as a substitute for a logical expression.
            - The Elements of Programming Style (Kernighan & Plauger)

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Wed, Nov 23, 2016 at 08:30:20AM +0100, Vincent Bernat wrote:
>  ❦ 22 novembre 2016 10:49 GMT, Stefan Hajnoczi <stefanha@gmail.com> :
> 
> >> I guess the question is that while the patch above could be accepted for
> >> the upcoming 2.8 (I don't see it breaking existing systems), a patch
> >> that would implement the transformation would be a lot more involved,
> >> and really not suitable for 2.8.
> >
> > This pull request cannot be accepted for QEMU 2.8 because it is not a
> > bug fix.  QEMU is in hard feature freeze (until mid-December):
> > http://qemu-project.org/Planning/HardFeatureFreeze
> > http://qemu-project.org/Planning/2.8
> 
> So, I'll implement netascii in the coming weeks and update the proposed
> patch.

Excellent, thank you!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[Vincent Bernat]

 ❦ 21 novembre 2016 14:46 GMT, Stefan Hajnoczi <stefanha@redhat.com> :

> The commit description says it would be "far more complex" to implement
> netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so hard?

Currently, the code uses lseek to send each block. It is just a matter
of doing nr_block*512. If there is a transformation, this cannot work
this way. But this can be done, we'll just have to maintain an offset.

Not knowing the final size is not a problem, just send back 0 as the
size.
-- 
Vincent Bernat — Vincent.Bernat@exoscale.ch
❬❱ https://www.exoscale.ch

Re: [Qemu-devel] [v2] tftp: fake support for netascii protocol

[hpa]

On November 21, 2016 7:35:28 AM PST, Vincent Bernat <Vincent.Bernat@exoscale.ch> wrote:
> ❦ 21 novembre 2016 14:46 GMT, Stefan Hajnoczi <stefanha@redhat.com> :
>
>> The commit description says it would be "far more complex" to
>implement
>> netascii.  Is the LF -> CR LF and CR -> CR NUL transformation so
>hard?
>
>Currently, the code uses lseek to send each block. It is just a matter
>of doing nr_block*512. If there is a transformation, this cannot work
>this way. But this can be done, we'll just have to maintain an offset.
>
>Not knowing the final size is not a problem, just send back 0 as the
>size.

Incidentally, it would definitely be a good thing to  enable the blksize extension.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Vincent Bernat]

 ❦ 19 novembre 2016 23:30 +0100, Samuel Thibault <samuel.thibault@gnu.org> :

>> > I think you should at least issue a qemu_log_mask(LOG_UNIMP, "...")
>> > call in that case.
>> 
>> I can do that if needed.
>
> That'd be better indeed. Otherwise people might wonder why things are
> not working. Warning that they have to do the LF -> CR,LF conversion by
> hand is important here.

I have sent an updated patch for that.

Totally unrelated, but on Debian, I have to use -r instead of -Wl,r for
LD_REL. This is explained here:
 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=837574

I don't think the patch has been forwarded to you yet.
-- 
Don't patch bad code - rewrite it.
            - The Elements of Programming Style (Kernighan & Plauger)

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

The following changes since commit b0bcc86d2a87456f5a276f941dc775b265b309cf:

  Update version for v2.8.0-rc0 release (2016-11-15 20:55:12 +0000)

are available in the git repository at:

  http://people.debian.org/~sthibault/qemu.git tags/samuel-thibault

for you to fetch changes up to b4a952793033e322f9ab0a3661b3faeee17ba4e6:

  tftp: fake support for netascii protocol (2016-11-20 18:02:38 +0100)

----------------------------------------------------------------
slirp updates

----------------------------------------------------------------
Vincent Bernat (1):
      tftp: fake support for netascii protocol

 slirp/tftp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

[Qemu-devel] [PULL] tftp: fake support for netascii protocol

[Samuel Thibault]

From: Vincent Bernat <vincent@bernat.im>

Some network equipments are requesting a file using the netascii
protocol and this is not configurable. Currently, qemu's tftpd only
supports the octet protocol. This commit makes it accept the netascii
protocol as well but do not perform the requested transformation (LF ->
CR,LF) as it would be far more complex. The current implementation is
good enough. A user has always the choice to preencode the served file
correctly.

Signed-off-by: Vincent Bernat <vincent@bernat.im>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 slirp/tftp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index c185906..6907d5b 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -26,6 +26,7 @@
 #include "slirp.h"
 #include "qemu-common.h"
 #include "qemu/cutils.h"
+#include "qemu/log.h"
 
 static inline int tftp_session_in_use(struct tftp_session *spt)
 {
@@ -326,13 +327,17 @@ static void tftp_handle_rrq(Slirp *slirp, struct sockaddr_storage *srcsas,
     return;
   }
 
-  if (strcasecmp(&tp->x.tp_buf[k], "octet") != 0) {
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "
+                    "no CR-LF conversion\n");
+      k += 9;
+  } else {
       tftp_send_error(spt, 4, "Unsupported transfer mode", tp);
       return;
   }
 
-  k += 6; /* skipping octet */
-
   /* do sanity checks on the filename */
   if (!strncmp(req_fname, "../", 3) ||
       req_fname[strlen(req_fname) - 1] == '/' ||
-- 
2.10.2

Re: [Qemu-devel] [PULL] tftp: fake support for netascii protocol

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PULL] tftp: fake support for netascii protocol
Type: series
Message-id: 20161120170517.9181-2-samuel.thibault@ens-lyon.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20161120170517.9181-2-samuel.thibault@ens-lyon.org -> patchew/20161120170517.9181-2-samuel.thibault@ens-lyon.org
Switched to a new branch 'test'
e9a07af tftp: fake support for netascii protocol

=== OUTPUT BEGIN ===
Checking PATCH 1/1: tftp: fake support for netascii protocol...
ERROR: suspect code indent for conditional statements (2, 6)
#35: FILE: slirp/tftp.c:330:
+  if (strcasecmp(&tp->x.tp_buf[k], "octet") == 0) {
+      k += 6;

ERROR: suspect code indent for conditional statements (2, 6)
#37: FILE: slirp/tftp.c:332:
+  } else if (strcasecmp(&tp->x.tp_buf[k], "netascii") == 0) {
+      qemu_log_mask(LOG_UNIMP, "tftp: netascii protocol not implemented, "

total: 2 errors, 0 warnings, 27 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org