Re: [PATCH v1 1/1] chardev: enable guest socket status/crontrol via DTR and DCD

["Darrin M. Gorski" <darrin@gorski.net>]

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

> That sounds like a good idea to me, but others may have different
opinions.

My original idea was simply to allow DCD to track socket state, because
this is what I need.  The DTR idea came from the referenced bug.  The
feature defaults to disabled like many of the other edge cases (like telnet
and tn3270), and it's a reasonably small non-intrusive change.

> Can we make this generic over all chardevs by moving it to the base class?

I'll take a closer look at the base class. I admit I did not spend much
time studying it.  This seemed like a socket specific feature to me.  It
seems like it might conflict with other chardevs like pty and serial as
they are already using ioctl for interaction with real tty devices.

> This feature will need some new tests in tests/test-char.c.

I would be happy to add tests but would need some guidance.

- Darrin

On Thu, Dec 17, 2020 at 9:16 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Thu, Dec 17, 2020 at 2:54 AM Darrin M. Gorski <darrin@gorski.net>
> wrote:
>
>>
>> This patch adds a 'modemctl' option to "-chardev socket" to enable
>> control of the socket via the guest serial port.
>> The default state of the option is disabled.
>>
>> 1. disconnect a connected socket when DTR transitions to low, also reject
>> new connections while DTR is low.
>> 2. provide socket connection status through the carrier detect line (CD
>> or DCD) on the guest serial port
>>
>
> That sounds like a good idea to me, but others may have different opinions.
>
>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1213196
>>
>> Signed-off-by: Darrin M. Gorski <darrin@gorski.net>
>>
>>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index 213a4c8dd0..94dd28e0cd 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -36,6 +36,7 @@
>>  #include "qapi/qapi-visit-sockets.h"
>>
>>  #include "chardev/char-io.h"
>> +#include "chardev/char-serial.h"
>>  #include "qom/object.h"
>>
>>  /***********************************************************/
>> @@ -85,6 +86,9 @@ struct SocketChardev {
>>      bool connect_err_reported;
>>
>>      QIOTask *connect_task;
>> +
>> +    bool is_modemctl;
>> +    int modem_state;
>>
>
> Can we make this generic over all chardevs by moving it to the base class?
>
>  };
>>  typedef struct SocketChardev SocketChardev;
>>
>> @@ -98,12 +102,18 @@ static void tcp_chr_change_state(SocketChardev *s,
>> TCPChardevState state)
>>  {
>>      switch (state) {
>>      case TCP_CHARDEV_STATE_DISCONNECTED:
>> +        if(s->is_modemctl) {
>> +            s->modem_state &= ~(CHR_TIOCM_CAR);
>> +        }
>>          break;
>>      case TCP_CHARDEV_STATE_CONNECTING:
>>          assert(s->state == TCP_CHARDEV_STATE_DISCONNECTED);
>>          break;
>>      case TCP_CHARDEV_STATE_CONNECTED:
>>          assert(s->state == TCP_CHARDEV_STATE_CONNECTING);
>> +        if(s->is_modemctl) {
>> +            s->modem_state |= CHR_TIOCM_CAR;
>> +        }
>>          break;
>>      }
>>      s->state = state;
>> @@ -947,6 +957,12 @@ static void tcp_chr_accept(QIONetListener *listener,
>>      tcp_chr_change_state(s, TCP_CHARDEV_STATE_CONNECTING);
>>      tcp_chr_set_client_ioc_name(chr, cioc);
>>      tcp_chr_new_client(chr, cioc);
>> +
>> +    if(s->is_modemctl) {
>> +        if(!(s->modem_state & CHR_TIOCM_DTR)) {
>> +            tcp_chr_disconnect(chr); /* disconnect if DTR is low */
>> +        }
>> +    }
>>  }
>>
>>
>> @@ -1322,12 +1338,17 @@ static void qmp_chardev_open_socket(Chardev *chr,
>>      bool is_tn3270      = sock->has_tn3270  ? sock->tn3270  : false;
>>      bool is_waitconnect = sock->has_wait    ? sock->wait    : false;
>>      bool is_websock     = sock->has_websocket ? sock->websocket : false;
>> +    bool is_modemctl     = sock->has_modemctl ? sock->modemctl : false;
>>      int64_t reconnect   = sock->has_reconnect ? sock->reconnect : 0;
>>      SocketAddress *addr;
>>
>>      s->is_listen = is_listen;
>>      s->is_telnet = is_telnet;
>>      s->is_tn3270 = is_tn3270;
>> +    s->is_modemctl = is_modemctl;
>> +    if(is_modemctl) {
>> +      s->modem_state = CHR_TIOCM_CTS | CHR_TIOCM_DSR;
>> +    }
>>      s->is_websock = is_websock;
>>      s->do_nodelay = do_nodelay;
>>      if (sock->tls_creds) {
>> @@ -1448,6 +1469,8 @@ static void qemu_chr_parse_socket(QemuOpts *opts,
>> ChardevBackend *backend,
>>      sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
>>      sock->has_tls_authz = qemu_opt_get(opts, "tls-authz");
>>      sock->tls_authz = g_strdup(qemu_opt_get(opts, "tls-authz"));
>> +    sock->has_modemctl = qemu_opt_get(opts, "modemctl");
>> +    sock->modemctl = qemu_opt_get_bool(opts, "modemctl", false);
>>
>>      addr = g_new0(SocketAddressLegacy, 1);
>>      if (path) {
>> @@ -1501,6 +1524,51 @@ char_socket_get_connected(Object *obj, Error
>> **errp)
>>      return s->state == TCP_CHARDEV_STATE_CONNECTED;
>>  }
>>
>> +/* ioctl support: allow guest to control/track socket state
>> + * via modem control lines (DCD/DTR)
>> + */
>> +static int
>> +char_socket_ioctl(Chardev *chr, int cmd, void *arg)
>> +{
>> +    SocketChardev *s = SOCKET_CHARDEV(chr);
>> +
>> +    if(!(s->is_modemctl)) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    switch (cmd) {
>> +        case CHR_IOCTL_SERIAL_GET_TIOCM:
>> +            {
>> +                int *targ = (int *)arg;
>> +                *targ = s->modem_state;
>> +            }
>> +            break;
>> +        case CHR_IOCTL_SERIAL_SET_TIOCM:
>> +            {
>> +                int sarg = *(int *)arg;
>> +                if (sarg & CHR_TIOCM_RTS) {
>> +                    s->modem_state |= CHR_TIOCM_RTS;
>> +                } else {
>> +                    s->modem_state &= ~(CHR_TIOCM_RTS);
>> +                }
>> +                if (sarg & CHR_TIOCM_DTR) {
>> +                    s->modem_state |= CHR_TIOCM_DTR;
>> +                } else {
>> +                    s->modem_state &= ~(CHR_TIOCM_DTR);
>> +                    /* disconnect if DTR goes low */
>> +                    if(s->state == TCP_CHARDEV_STATE_CONNECTED) {
>> +                        tcp_chr_disconnect(chr);
>> +                    }
>> +                }
>> +            }
>> +            break;
>> +        default:
>> +            return -ENOTSUP;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void char_socket_class_init(ObjectClass *oc, void *data)
>>  {
>>      ChardevClass *cc = CHARDEV_CLASS(oc);
>> @@ -1516,6 +1584,7 @@ static void char_socket_class_init(ObjectClass *oc,
>> void *data)
>>      cc->chr_add_client = tcp_chr_add_client;
>>      cc->chr_add_watch = tcp_chr_add_watch;
>>      cc->chr_update_read_handler = tcp_chr_update_read_handler;
>> +    cc->chr_ioctl = char_socket_ioctl;
>>
>>      object_class_property_add(oc, "addr", "SocketAddress",
>>                                char_socket_get_addr, NULL,
>> diff --git a/chardev/char.c b/chardev/char.c
>> index a9b8c5a9aa..601d818f81 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -928,6 +928,9 @@ QemuOptsList qemu_chardev_opts = {
>>          },{
>>              .name = "logappend",
>>              .type = QEMU_OPT_BOOL,
>> +        },{
>> +            .name = "modemctl",
>> +            .type = QEMU_OPT_BOOL,
>>  #ifdef CONFIG_LINUX
>>          },{
>>              .name = "tight",
>> diff --git a/qapi/char.json b/qapi/char.json
>> index 58338ed62d..f352bd6350 100644
>> --- a/qapi/char.json
>> +++ b/qapi/char.json
>> @@ -271,6 +271,9 @@
>>  #             then attempt a reconnect after the given number of seconds.
>>  #             Setting this to zero disables this function. (default: 0)
>>  #             (Since: 2.2)
>> +# @modemctl: allow guest to use modem control signals to control/monitor
>> +#            the socket state (CD follows is_connected, DTR influences
>> +#            connect/accept) (default: false) (Since: 5.2)
>>  #
>>  # Since: 1.4
>>  ##
>> @@ -284,6 +287,7 @@
>>              '*telnet': 'bool',
>>              '*tn3270': 'bool',
>>              '*websocket': 'bool',
>> +            '*modemctl': 'bool',
>>              '*reconnect': 'int' },
>>    'base': 'ChardevCommon' }
>>
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 459c916d3d..f09072afbf 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -2991,11 +2991,13 @@ DEFHEADING(Character device options:)
>>  DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>>      "-chardev help\n"
>>      "-chardev
>> null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>> -    "-chardev
>> socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay][,reconnect=seconds]\n"
>> -    "
>> [,server][,nowait][,telnet][,websocket][,reconnect=seconds][,mux=on|off]\n"
>> -    "
>> [,logfile=PATH][,logappend=on|off][,tls-creds=ID][,tls-authz=ID] (tcp)\n"
>> -    "-chardev
>> socket,id=id,path=path[,server][,nowait][,telnet][,websocket][,reconnect=seconds]\n"
>> -    "
>> [,mux=on|off][,logfile=PATH][,logappend=on|off][,abstract=on|off][,tight=on|off]
>> (unix)\n"
>> +    "-chardev
>> socket,id=id[,host=host],port=port[,to=to][,ipv4][,ipv6][,nodelay]\n"
>> +    "
>> [,reconnect=seconds][,server][,nowait][,telnet][,websocket]\n"
>> +    "
>> [,modemctl][,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>> +    "         [,tls-creds=ID][,tls-authz=ID] (tcp)\n"
>> +    "-chardev
>> socket,id=id,path=path[,server][,nowait][,telnet][,websocket]\n"
>> +    "
>> [,reconnect=seconds][,modemctl][,mux=on|off][,logfile=PATH]\n"
>> +    "         [,logappend=on|off][,abstract=on|off][,tight=on|off]
>> (unix)\n"
>>      "-chardev udp,id=id[,host=host],port=port[,localaddr=localaddr]\n"
>>      "         [,localport=localport][,ipv4][,ipv6][,mux=on|off]\n"
>>      "         [,logfile=PATH][,logappend=on|off]\n"
>>
>
> This feature will need some new tests in tests/test-char.c.
>
> --
> Marc-André Lureau
>

[-- Attachment #2: Type: text/html, Size: 13129 bytes --]