[PULL,1/9] qapi: Add InetSocketAddress member keep-alive

Commit Message

Eric Blake Aug. 15, 2019, 6:30 p.m. UTC

From: Vladimir Sementsov-Ogievskiy <[e-mail]>

It's needed to provide keepalive for nbd client to track server
availability.

Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Markus Armbruster <[e-mail]>
Acked-by: Daniel P. Berrangé <[e-mail]>
[eblake: Fix error message typo]
Signed-off-by: Eric Blake <[e-mail]>
---
 qapi/sockets.json   |  6 +++++-
 util/qemu-sockets.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

Comments

Peter Maydell Sept. 9, 2019, 5:32 p.m. UTC | #1

On Thu, 15 Aug 2019 at 19:34, Eric Blake <[e-mail]> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <[e-mail]>
>
> It's needed to provide keepalive for nbd client to track server
> availability.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> Message-Id: <[e-mail]>
> Reviewed-by: Markus Armbruster <[e-mail]>
> Acked-by: Daniel P. Berrangé <[e-mail]>
> [eblake: Fix error message typo]
> Signed-off-by: Eric Blake <[e-mail]>

Hi; Coverity spots a bug in this change (CID 1405300):

> @@ -458,6 +464,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>      }

At this point in the function, we might be in one of
two cases:
 (1) sock >= 0 : the connect succeeded
 (2) sock < 0 : connect failed, we have just called
     error_propagate() and are using the same end-of-function
     codepath to free 'res' before returning the error code.

>
>      freeaddrinfo(res);
> +
> +    if (saddr->keep_alive) {
> +        int val = 1;
> +        int ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
> +                                  &val, sizeof(val));

...but here we use 'sock' assuming it is valid.

> +
> +        if (ret < 0) {
> +            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
> +            close(sock);
> +            return -1;
> +        }
> +    }
> +
>      return sock;
>  }

If we want to add more "actual work" at this point in
the function we should probably separate out the failed-case
codepath, eg by changing


    if (sock < 0) {
        error_propagate(errp, local_err);
    }

    freeaddrinfo(res);

into

    freeaddrinfo(res);

    if (sock < 0) {
        error_propagate(errp, local_err);
        return sock;
    }



thanks
-- PMM

Vladimir Sementsov-Ogievskiy Sept. 10, 2019, 7:56 a.m. UTC | #2

09.09.2019 20:32, Peter Maydell wrote:
> On Thu, 15 Aug 2019 at 19:34, Eric Blake <[e-mail]> wrote:
>>
>> From: Vladimir Sementsov-Ogievskiy <[e-mail]>
>>
>> It's needed to provide keepalive for nbd client to track server
>> availability.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>> Message-Id: <[e-mail]>
>> Reviewed-by: Markus Armbruster <[e-mail]>
>> Acked-by: Daniel P. Berrangé <[e-mail]>
>> [eblake: Fix error message typo]
>> Signed-off-by: Eric Blake <[e-mail]>
> 
> Hi; Coverity spots a bug in this change (CID 1405300):

Two coverity bugs on my patches, I'm sorry :(

How can I run this coverity locally?

> 
>> @@ -458,6 +464,19 @@ int inet_connect_saddr(InetSocketAddress *saddr, Error **errp)
>>       }
> 
> At this point in the function, we might be in one of
> two cases:
>   (1) sock >= 0 : the connect succeeded
>   (2) sock < 0 : connect failed, we have just called
>       error_propagate() and are using the same end-of-function
>       codepath to free 'res' before returning the error code.
> 
>>
>>       freeaddrinfo(res);
>> +
>> +    if (saddr->keep_alive) {
>> +        int val = 1;
>> +        int ret = qemu_setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE,
>> +                                  &val, sizeof(val));
> 
> ...but here we use 'sock' assuming it is valid.
> 
>> +
>> +        if (ret < 0) {
>> +            error_setg_errno(errp, errno, "Unable to set KEEPALIVE");
>> +            close(sock);
>> +            return -1;
>> +        }
>> +    }
>> +
>>       return sock;
>>   }
> 
> If we want to add more "actual work" at this point in
> the function we should probably separate out the failed-case
> codepath, eg by changing
> 
> 
>      if (sock < 0) {
>          error_propagate(errp, local_err);
>      }
> 
>      freeaddrinfo(res);
> 
> into
> 
>      freeaddrinfo(res);
> 
>      if (sock < 0) {
>          error_propagate(errp, local_err);
>          return sock;
>      }
> 
> 
> 
> thanks
> -- PMM
> 

Thanks, this should work, I'll send a patch soon.

Peter Maydell Sept. 10, 2019, 8:10 a.m. UTC | #3

On Tue, 10 Sep 2019 at 08:56, Vladimir Sementsov-Ogievskiy
<[e-mail]> wrote:
>
> 09.09.2019 20:32, Peter Maydell wrote:
> > On Thu, 15 Aug 2019 at 19:34, Eric Blake <[e-mail]> wrote:
> >>
> >> From: Vladimir Sementsov-Ogievskiy <[e-mail]>
> >>
> >> It's needed to provide keepalive for nbd client to track server
> >> availability.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> >> Message-Id: <[e-mail]>
> >> Reviewed-by: Markus Armbruster <[e-mail]>
> >> Acked-by: Daniel P. Berrangé <[e-mail]>
> >> [eblake: Fix error message typo]
> >> Signed-off-by: Eric Blake <[e-mail]>
> >
> > Hi; Coverity spots a bug in this change (CID 1405300):
>
> Two coverity bugs on my patches, I'm sorry :(
>
> How can I run this coverity locally?

You can't -- it's the online free 'Coverity Scan' service
that Coverity provide for open source projects, and it
only runs on master once patches have been committed.
So we sort of expect that it will catch some of these minor
things after the fact -- it's not a problem.

(I guess if you happen to own a copy of the commercial
Coverity product it is possible to do a local scan but I
wouldn't know how.)

thanks
-- PMM

Patch

diff --git a/qapi/sockets.json b/qapi/sockets.json
index fc81d8d5e8b9..32375f3a361e 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 passive sockets. (Since 4.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 a5092dbd12e7..e3a1666578d9 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -219,6 +219,12 @@  static int inet_listen_saddr(InetSocketAddress *saddr,
     bool socket_created = false;
     Error *err = NULL;

+    if (saddr->keep_alive) {
+        error_setg(errp, "keep-alive option is not supported for passive "
+                   "sockets");
+        return -1;
+    }
+
     memset(&ai,0, sizeof(ai));
     ai.ai_flags = AI_PASSIVE;
     if (saddr->has_numeric && saddr->numeric) {
@@ -458,6 +464,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;
 }

@@ -653,6 +672,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;
 }