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

[Samuel Thibault <samuel.thibault@gnu.org>]

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