Re: [Qemu-devel] [PATCH v2 for-2.10 1/8] chardev: Basic support for TN3270

[Cornelia Huck]

On Fri, 7 Apr 2017 08:58:15 -0500
Eric Blake <address@hidden> wrote:

> On 04/07/2017 06:18 AM, Cornelia Huck wrote:
> > From: Jing Liu <address@hidden>
> > 
> > This introduces basic support for TN3270, which needs to negotiate
> > three Telnet options during handshake:
> >   - End of Record
> >   - Binary Transmission
> >   - Terminal-Type
> > 
> > As a basic implementation, this simply ignores NOP and Interrupt
> > Process(IP) commands. More work should be done for them later.
> > 
> > For more details, please refer to RFC 854 and 1576.
> > 
> > Signed-off-by: Jing Liu <address@hidden>
> > Signed-off-by: Yang Chen <address@hidden>
> > Reviewed-by: QingFeng Hao <address@hidden>
> > Acked-by: Dong Jia Shi <address@hidden>
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -4874,6 +4874,8 @@
> >  # @nodelay: set TCP_NODELAY socket option (default: false)
> >  # @telnet: enable telnet protocol on server
> >  #          sockets (default: false)
> > +# @tn3270: enable tn3270 protocol on server
> > +#          sockets (default: false) (Since: 2.10)
> >  # @reconnect: For a client socket, if a socket is disconnected,
> >  #          then attempt a reconnect after the given number of seconds.
> >  #          Setting this to zero disables this function. (default: 0)
> > @@ -4887,6 +4889,7 @@
> >                                       '*wait'      : 'bool',
> >                                       '*nodelay'   : 'bool',
> >                                       '*telnet'    : 'bool',
> > +                                     '*tn3270'    : 'bool',
> >                                       '*reconnect' : 'int' },
> >    'base': 'ChardevCommon' }
> 
> I almost wonder if we should make this a flat union, so that members
> that only make sense with the server side (such as tn3270) or with the
> client side (reconnect) can only be present when that branch of the
> union is taken.  But that may break back-compat if we have existing code
> that silently ignores a member that makes no sense, and we start
> forcefully warning about the member being present.  

The existing mix of server and client side members is a bit
unfortunate, but I'm not sure it's worth spending effort on this.
Anyway, I'd defer that to the chardev maintainers :)

> At any rate, making
> such a change is not a prerequisite for this series.

Would be nice if this is fine as-is.