| Message ID | 20161120084136.721-1-Vincent.Bernat@exoscale.ch |
|---|---|
| State | New |
| Headers | show |
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
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
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>
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
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.
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
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.
❦ 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.
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.
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
❦ 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.
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
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] == '/' ||