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: 12048 bytes --]

>> Yes, but they can override the default behaviour, or just reuse some
code.
>> If it doesn't fit, don't worry about it, we can just start with socket.

Whew.  I spent several hours yesterday trying to figure out how to alter
the base class to support this - DCD could be reasonably simple, but DTR is
another matter.  I will continue to look at that, but I do think moving
forward with sockets (since this patch already works that way) is a good
compromise.

> You can look at char_socket_server_test.

Will do.

- Darrin

On Fri, Dec 18, 2020 at 6:57 AM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Thu, Dec 17, 2020 at 9:54 PM Darrin M. Gorski <darrin@gorski.net>
> wrote:
>
>>
>> > 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.
>>
>
> Yes, but they can override the default behaviour, or just reuse some code.
>
> If it doesn't fit, don't worry about it, we can just start with socket.
>
>
>> > This feature will need some new tests in tests/test-char.c.
>>
>> I would be happy to add tests but would need some guidance.
>>
>
> You can look at char_socket_server_test. You can extend
> CharSocketServerTestConfig to check for modemctl behaviour. It will need to
> send the ioctl with qemu_chr_fe_ioctl (I admit it wasn't tested so far,
> because only serial and parallel chardev did implement it, and it's not
> easy to test those)
>
> Let me know if you need more help
>
>
>> - 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
>>>
>>
>
> --
> Marc-André Lureau
>

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