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
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.
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
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.
© 2016 - 2024 Red Hat, Inc.