[PATCH 6/6] tools/libxs: Stop playing with SIGPIPE

Andrew Cooper posted 6 patches 2 months ago
[PATCH 6/6] tools/libxs: Stop playing with SIGPIPE
Posted by Andrew Cooper 2 months ago
It's very rude for a library to play with signals behind the back of the
application, no matter ones views on the default behaviour of SIGPIPE under
POSIX.  Even if the application doesn't care about the xenstored socket, it my
care about others.

This logic has existed since xenstore/xenstored was originally added in commit
29c9e570b1ed ("Add xenstore daemon and library") in 2005.

It's also unnecessary.  Pass MSG_NOSIGNAL when talking to xenstored over a
pipe (to avoid sucumbing to SIGPIPE if xenstored has crashed), and forgo any
playing with the signal disposition.

This has a side benefit of saving 2 syscalls per xenstore request.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@vates.tech>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libs/store/xs.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 9db5c02661f5..63d754e17014 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -579,7 +579,7 @@ static bool sendmsg_exact(int fd, struct iovec *iov, unsigned int nr)
 	assert(iov->iov_len == sizeof(struct xsd_sockmsg));
 
 	while (hdr.msg_iovlen) {
-		ssize_t res = sendmsg(fd, &hdr, 0);
+		ssize_t res = sendmsg(fd, &hdr, MSG_NOSIGNAL);
 
 		if (res < 0 && errno == EINTR)
 			continue;
@@ -680,7 +680,6 @@ static void *xs_talkv(struct xs_handle *h,
 	void *ret = NULL;
 	int saved_errno;
 	unsigned int i, msg_len;
-	struct sigaction ignorepipe, oldact;
 
 	/* Element 0 must be xsd_sockmsg */
 	assert(num_vecs >= 1);
@@ -697,11 +696,6 @@ static void *xs_talkv(struct xs_handle *h,
 
 	msg->len = msg_len;
 
-	ignorepipe.sa_handler = SIG_IGN;
-	sigemptyset(&ignorepipe.sa_mask);
-	ignorepipe.sa_flags = 0;
-	sigaction(SIGPIPE, &ignorepipe, &oldact);
-
 	mutex_lock(&h->request_mutex);
 
 	if (!write_request(h, iovec, num_vecs))
@@ -713,7 +707,6 @@ static void *xs_talkv(struct xs_handle *h,
 
 	mutex_unlock(&h->request_mutex);
 
-	sigaction(SIGPIPE, &oldact, NULL);
 	if (reply_type == XS_ERROR) {
 		saved_errno = get_error(ret);
 		free(ret);
@@ -732,7 +725,6 @@ static void *xs_talkv(struct xs_handle *h,
 	/* 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;
-- 
2.39.2
Re: [PATCH 6/6] tools/libxs: Stop playing with SIGPIPE
Posted by Jason Andryuk 1 month, 3 weeks ago
On 2024-07-18 12:48, Andrew Cooper wrote:
> It's very rude for a library to play with signals behind the back of the
> application, no matter ones views on the default behaviour of SIGPIPE under
> POSIX.  Even if the application doesn't care about the xenstored socket, it my
> care about others.
> 
> This logic has existed since xenstore/xenstored was originally added in commit
> 29c9e570b1ed ("Add xenstore daemon and library") in 2005.
> 
> It's also unnecessary.  Pass MSG_NOSIGNAL when talking to xenstored over a
> pipe (to avoid sucumbing to SIGPIPE if xenstored has crashed), and forgo any
> playing with the signal disposition.
> 
> This has a side benefit of saving 2 syscalls per xenstore request.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Re: [PATCH 6/6] tools/libxs: Stop playing with SIGPIPE
Posted by Jürgen Groß 1 month, 3 weeks ago
On 18.07.24 18:48, Andrew Cooper wrote:
> It's very rude for a library to play with signals behind the back of the
> application, no matter ones views on the default behaviour of SIGPIPE under
> POSIX.  Even if the application doesn't care about the xenstored socket, it my
> care about others.
> 
> This logic has existed since xenstore/xenstored was originally added in commit
> 29c9e570b1ed ("Add xenstore daemon and library") in 2005.
> 
> It's also unnecessary.  Pass MSG_NOSIGNAL when talking to xenstored over a
> pipe (to avoid sucumbing to SIGPIPE if xenstored has crashed), and forgo any
> playing with the signal disposition.
> 
> This has a side benefit of saving 2 syscalls per xenstore request.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen