1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v3] qapi: Add InetSocketAddress member keep-alive

Vladimir Sementsov-Ogievskiy posted 1 patch 4 years, 10 months ago
Test s390x passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190614173140.19159-1-vsementsov@virtuozzo.com
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>
There is a newer version of this series
qapi/sockets.json   |  6 +++++-
util/qemu-sockets.c | 28 ++++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH v3] qapi: Add InetSocketAddress member keep-alive
Posted by Vladimir Sementsov-Ogievskiy 4 years, 10 months ago 
It's needed to provide keepalive for nbd client to track server
availability.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

v3: [by Markus's comments]

- Fix commit subject
- Add comment to qapi and restrict server-side connections
- Fix s/"keep-alive="/",keep-alive"/


 qapi/sockets.json   |  6 +++++-
 util/qemu-sockets.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/qapi/sockets.json b/qapi/sockets.json
index fc81d8d5e8..c44af481a1 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -53,6 +53,9 @@
 #
 # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
 #
+# @keep-alive: enable keep-alive when connecting to this socket. Not supported
+#              for server-side connections. (Since 4.1)
+#
 # Since: 1.3
 ##
 { 'struct': 'InetSocketAddress',
@@ -61,7 +64,8 @@
     '*numeric':  'bool',
     '*to': 'uint16',
     '*ipv4': 'bool',
-    '*ipv6': 'bool' } }
+    '*ipv6': 'bool',
+    '*keep-alive': 'bool' } }
 
 ##
 # @UnixSocketAddress:
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8850a280a8..813063761b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -438,6 +438,12 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
     struct addrinfo *res, *e;
     int sock = -1;
 
+    if (saddr->keep_alive) {
+        error_setg(errp, "keep-alive options is not supported for server-side "
+                   "connection");
+        return -1;
+    }
+
     res = inet_parse_connect_saddr(saddr, errp);
     if (!res) {
         return -1;
@@ -457,6 +463,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
     }
 
     freeaddrinfo(res);
+
+    if (saddr->keep_alive) {
+        int val = 1;
+        int ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
+                                  &val, sizeof(val));
+
+        if (ret < 0) {
+            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
+            close(sock);
+            return -1;
+        }
+    }
+
     return sock;
 }
 
@@ -652,6 +671,15 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
         }
         addr->has_ipv6 = true;
     }
+    begin = strstr(optstr, ",keep-alive");
+    if (begin) {
+        if (inet_parse_flag("keep-alive", begin + strlen(",keep-alive"),
+                            &addr->keep_alive, errp) < 0)
+        {
+            return -1;
+        }
+        addr->has_keep_alive = true;
+    }
     return 0;
 }
 
-- 
2.18.0
Re: [Qemu-devel] [PATCH v3] qapi: Add InetSocketAddress member keep-alive
Posted by Markus Armbruster 4 years, 9 months ago 
I apologize for dragging my feet on this review.

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> It's needed to provide keepalive for nbd client to track server
> availability.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> v3: [by Markus's comments]
>
> - Fix commit subject
> - Add comment to qapi and restrict server-side connections
> - Fix s/"keep-alive="/",keep-alive"/
>
>
>  qapi/sockets.json   |  6 +++++-
>  util/qemu-sockets.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index fc81d8d5e8..c44af481a1 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -53,6 +53,9 @@
>  #
>  # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
>  #
> +# @keep-alive: enable keep-alive when connecting to this socket. Not supported
> +#              for server-side connections. (Since 4.1)
> +#

Is "server-side connection" is an established term?

For what it's worth, "passive socket" is, see listen(2).

>  # Since: 1.3
>  ##
>  { 'struct': 'InetSocketAddress',
> @@ -61,7 +64,8 @@
>      '*numeric':  'bool',
>      '*to': 'uint16',
>      '*ipv4': 'bool',
> -    '*ipv6': 'bool' } }
> +    '*ipv6': 'bool',
> +    '*keep-alive': 'bool' } }
>  
>  ##
>  # @UnixSocketAddress:
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8850a280a8..813063761b 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -438,6 +438,12 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>      struct addrinfo *res, *e;
>      int sock = -1;
>  
> +    if (saddr->keep_alive) {
> +        error_setg(errp, "keep-alive options is not supported for server-side "
> +                   "connection");
> +        return -1;
> +    }
> +
>      res = inet_parse_connect_saddr(saddr, errp);
>      if (!res) {
>          return -1;

I'm afraid you added this to the wrong function; ...

> @@ -457,6 +463,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>      }
>  
>      freeaddrinfo(res);
> +
> +    if (saddr->keep_alive) {

... it renders this code unreachable.

I guess the "not supported for passive sockets" check should go into
inet_listen_saddr() instead.

> +        int val = 1;
> +        int ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> +                                  &val, sizeof(val));
> +
> +        if (ret < 0) {
> +            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> +            close(sock);
> +            return -1;
> +        }
> +    }
> +
>      return sock;
>  }
>  
> @@ -652,6 +671,15 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
>          }
>          addr->has_ipv6 = true;
>      }
> +    begin = strstr(optstr, ",keep-alive");
> +    if (begin) {
> +        if (inet_parse_flag("keep-alive", begin + strlen(",keep-alive"),
> +                            &addr->keep_alive, errp) < 0)
> +        {
> +            return -1;
> +        }
> +        addr->has_keep_alive = true;
> +    }
>      return 0;
>  }
Re: [Qemu-devel] [PATCH v3] qapi: Add InetSocketAddress member keep-alive
Posted by Eric Blake 4 years, 9 months ago 
On 6/25/19 8:32 AM, Markus Armbruster wrote:
> I apologize for dragging my feet on this review.
> 
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> It's needed to provide keepalive for nbd client to track server
>> availability.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>

>> +++ b/qapi/sockets.json
>> @@ -53,6 +53,9 @@
>>  #
>>  # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
>>  #
>> +# @keep-alive: enable keep-alive when connecting to this socket. Not supported
>> +#              for server-side connections. (Since 4.1)

It looks like this missed 4.1.  Are you planning on sending v4, to address

>> +#
> 
> Is "server-side connection" is an established term?
> 
> For what it's worth, "passive socket" is, see listen(2).
> 
>>  # Since: 1.3
>>  ##
>>  { 'struct': 'InetSocketAddress',
>> @@ -61,7 +64,8 @@
>>      '*numeric':  'bool',
>>      '*to': 'uint16',
>>      '*ipv4': 'bool',
>> -    '*ipv6': 'bool' } }
>> +    '*ipv6': 'bool',
>> +    '*keep-alive': 'bool' } }
>>  
>>  ##
>>  # @UnixSocketAddress:
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 8850a280a8..813063761b 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -438,6 +438,12 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>>      struct addrinfo *res, *e;
>>      int sock = -1;
>>  
>> +    if (saddr->keep_alive) {
>> +        error_setg(errp, "keep-alive options is not supported for server-side "
>> +                   "connection");
>> +        return -1;
>> +    }
>> +
>>      res = inet_parse_connect_saddr(saddr, errp);
>>      if (!res) {
>>          return -1;
> 
> I'm afraid you added this to the wrong function; ...
> 
>> @@ -457,6 +463,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>>      }
>>  
>>      freeaddrinfo(res);
>> +
>> +    if (saddr->keep_alive) {
> 
> ... it renders this code unreachable.
> 
> I guess the "not supported for passive sockets" check should go into
> inet_listen_saddr() instead.

this comment?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v3] qapi: Add InetSocketAddress member keep-alive
Posted by Vladimir Sementsov-Ogievskiy 4 years, 8 months ago 
19.07.2019 23:29, Eric Blake wrote:
> On 6/25/19 8:32 AM, Markus Armbruster wrote:
>> I apologize for dragging my feet on this review.
>>
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> It's needed to provide keepalive for nbd client to track server
>>> availability.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>
> 
>>> +++ b/qapi/sockets.json
>>> @@ -53,6 +53,9 @@
>>>   #
>>>   # @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
>>>   #
>>> +# @keep-alive: enable keep-alive when connecting to this socket. Not supported
>>> +#              for server-side connections. (Since 4.1)
> 
> It looks like this missed 4.1.  Are you planning on sending v4, to address
> 
>>> +#
>>
>> Is "server-side connection" is an established term?
>>
>> For what it's worth, "passive socket" is, see listen(2).
>>
>>>   # Since: 1.3
>>>   ##
>>>   { 'struct': 'InetSocketAddress',
>>> @@ -61,7 +64,8 @@
>>>       '*numeric':  'bool',
>>>       '*to': 'uint16',
>>>       '*ipv4': 'bool',
>>> -    '*ipv6': 'bool' } }
>>> +    '*ipv6': 'bool',
>>> +    '*keep-alive': 'bool' } }
>>>   
>>>   ##
>>>   # @UnixSocketAddress:
>>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>>> index 8850a280a8..813063761b 100644
>>> --- a/util/qemu-sockets.c
>>> +++ b/util/qemu-sockets.c
>>> @@ -438,6 +438,12 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>>>       struct addrinfo *res, *e;
>>>       int sock = -1;
>>>   
>>> +    if (saddr->keep_alive) {
>>> +        error_setg(errp, "keep-alive options is not supported for server-side "
>>> +                   "connection");
>>> +        return -1;
>>> +    }
>>> +
>>>       res = inet_parse_connect_saddr(saddr, errp);
>>>       if (!res) {
>>>           return -1;
>>
>> I'm afraid you added this to the wrong function; ...
>>
>>> @@ -457,6 +463,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>>>       }
>>>   
>>>       freeaddrinfo(res);
>>> +
>>> +    if (saddr->keep_alive) {
>>
>> ... it renders this code unreachable.
>>
>> I guess the "not supported for passive sockets" check should go into
>> inet_listen_saddr() instead.
> 
> this comment?
> 

Sorry for big delay, will resend for 4.2 soon.

-- 
Best regards,
Vladimir

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.