[PULL] tftp: fake support for netascii protocol

Commit Message

Samuel Thibault Nov. 18, 2016, 5:51 p.m. UTC

From: Vincent Bernat <[e-mail]>

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 <[e-mail]>
Signed-off-by: Samuel Thibault <[e-mail]>
---
 slirp/tftp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

[e-mail] Nov. 18, 2016, 5:55 p.m. UTC | #1

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: [e-mail]

=== 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/[e-mail] -> patchew/[e-mail]
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 [e-mail]

Thomas Huth Nov. 19, 2016, 7:32 a.m. UTC | #2

On 18.11.2016 18:51, Samuel Thibault wrote:
> From: Vincent Bernat <[e-mail]>
> 
> 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.

Vincent Bernat Nov. 19, 2016, 8:03 a.m. UTC | #3

❦ 19 novembre 2016 08:32 +0100, Thomas Huth <[e-mail]> :

>> 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.

Samuel Thibault Nov. 19, 2016, 10:30 p.m. UTC | #4

Vincent Bernat, on Sat 19 Nov 2016 09:03:08 +0100, wrote:
>  ❦ 19 novembre 2016 08:32 +0100, Thomas Huth <[e-mail]> :
> 
> >> 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

Vincent Bernat Nov. 20, 2016, 8:42 a.m. UTC | #5

❦ 19 novembre 2016 23:30 +0100, Samuel Thibault <[e-mail]> :

>> > 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.

Patch

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] == '/' ||