[Xen-devel] [PATCH] tools/xenstore: don't close connection in xs_talkv()

Juergen Gross posted 1 patch 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200323142900.22255-1-jgross@suse.com
tools/xenstore/xs.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)

[Xen-devel] [PATCH] tools/xenstore: don't close connection in xs_talkv()

Posted by Juergen Gross 2 weeks ago
In case of some errors xs_talkv() will close the connection to
Xenstore. This is annoying as it is not clear to the caller in which
error case the connection is still available.

Drop that implicit closing to make the interface behave in a sane and
predictable way.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/xenstore/xs.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index aa1d24b8b9..bbf3e00bed 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -524,12 +524,14 @@ static void *xs_talkv(struct xs_handle *h, xs_transaction_t t,
 			goto fail;
 
 	ret = read_reply(h, &msg.type, len);
-	if (!ret)
-		goto fail;
 
 	mutex_unlock(&h->request_mutex);
 
 	sigaction(SIGPIPE, &oldact, NULL);
+
+	if (!ret)
+		return NULL;
+
 	if (msg.type == XS_ERROR) {
 		saved_errno = get_error(ret);
 		free(ret);
@@ -539,19 +541,15 @@ static void *xs_talkv(struct xs_handle *h, xs_transaction_t t,
 
 	if (msg.type != type) {
 		free(ret);
-		saved_errno = EBADF;
-		goto close_fd;
+		errno = EBADF;
+		return NULL;
 	}
 	return ret;
 
 fail:
-	/* We're in a bad state, so close fd. */
 	saved_errno = errno;
 	mutex_unlock(&h->request_mutex);
 	sigaction(SIGPIPE, &oldact, NULL);
-close_fd:
-	close(h->fd);
-	h->fd = -1;
 	errno = saved_errno;
 	return NULL;
 }
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] tools/xenstore: don't close connection in xs_talkv()

Posted by Andrew Cooper 2 weeks ago
On 23/03/2020 14:29, Juergen Gross wrote:
> In case of some errors xs_talkv() will close the connection to
> Xenstore. This is annoying as it is not clear to the caller in which
> error case the connection is still available.
>
> Drop that implicit closing to make the interface behave in a sane and
> predictable way.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This definitely does improve the cascade failure cases.

Re: [Xen-devel] [PATCH] tools/xenstore: don't close connection in xs_talkv()

Posted by Andrew Cooper 2 weeks ago
On 23/03/2020 15:44, Andrew Cooper wrote:
> On 23/03/2020 14:29, Juergen Gross wrote:
>> In case of some errors xs_talkv() will close the connection to
>> Xenstore. This is annoying as it is not clear to the caller in which
>> error case the connection is still available.
>>
>> Drop that implicit closing to make the interface behave in a sane and
>> predictable way.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> This definitely does improve the cascade failure cases.
>

Actually, I spoke too soon.  The EBADF goes, but the next read_message()
ends up pulling junk out of the ring.

I think something else in the middle needs teaching about hard errors,
and not to pull more data out of the ring.

~Andrew

Re: [Xen-devel] [PATCH] tools/xenstore: don't close connection in xs_talkv()

Posted by Ian Jackson 2 weeks ago
Sorry to come into this late.

Andrew Cooper writes ("Re: [Xen-devel] [PATCH] tools/xenstore: don't close connection in xs_talkv()"):
> On 23/03/2020 15:44, Andrew Cooper wrote:
> > On 23/03/2020 14:29, Juergen Gross wrote:
> >> In case of some errors xs_talkv() will close the connection to
> >> Xenstore. This is annoying as it is not clear to the caller in which
> >> error case the connection is still available.
> >>
> >> Drop that implicit closing to make the interface behave in a sane and
> >> predictable way.
> >>
> >> Signed-off-by: Juergen Gross <jgross@suse.com>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >
> > This definitely does improve the cascade failure cases.
> 
> Actually, I spoke too soon.  The EBADF goes, but the next read_message()
> ends up pulling junk out of the ring.

I'm afraid this is predictable.  For most of these errors there is
nothing else sensible that the client library could do.  The
connection has been rendered unuseable.  In principle I guess it could
reconnect to the socket, but this is a "should never happend" event.

EBADF is a worrying error because in many cases it means something has
messed up the process's fd space, which can easily lead to really bad
behaviours.

But having read the code in xenstore-ls, I see that as well as closing
the fd it sets the variable containing the fd number to -1.  So all
future calls return EBADF.

I think this is correct.

Ian.