tools/libxs: Fix SIGPIPE handling issues

[PATCH 0/6] tools/libxs: Fix SIGPIPE handling issues

Posted by Andrew Cooper 1 month, 3 weeks ago

While the purpose of this series is patch 6, it has a side effect of reducing
the number of system calls substantally.

Using a strace of test-xenstore as an example, we go from this:

  rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, 8) = 0
  write(3, "\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", 16) = 16
  write(3, "xenstore-test/502673/a\0", 23) = 23
  write(3, "a", 1)                        = 1
  read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16
  read(3, "OK\0", 3)                      = 3
  rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, NULL, 8) = 0

down to this:

  sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", iov_len=16}, {iov_base="xenstore-test/504021/a\0", iov_len=23}, {iov_base="a", iov_len=1}], msg_iovlen=3, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 40
  read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16
  read(3, "OK\0", 3)                      = 3


I.e., it removes 2x rt_sigaction(), and turns all write()'s into a single
writev() or sendmsg().


Reads are a little more problematic to deal with.  Xenstored will produce a
full package basically in one go, but libxenstore's reading is horrbly
complicated by virtue of it being completely different depending on whether
xs_watch() has created a secondary read thread or not.

Andrew Cooper (6):
  tools/libxs: Fix length check in xs_talkv()
  tools/libxs: Rework xs_talkv() to take xsd_sockmsg within the iovec
  tools/libxs: Rationalise the definition of struct xs_handle
  tools/libxs: Track whether we're using a socket or file
  tools/libxs: Use writev()/sendmsg() instead of write()
  tools/libxs: Stop playing with SIGPIPE

 tools/libs/store/xs.c | 394 +++++++++++++++++++++++++++---------------
 1 file changed, 258 insertions(+), 136 deletions(-)

-- 
2.39.2

Re: [PATCH 0/6] tools/libxs: Fix SIGPIPE handling issues

Posted by Jan Beulich 1 month, 2 weeks ago

On 18.07.2024 18:48, Andrew Cooper wrote:
> While the purpose of this series is patch 6, it has a side effect of reducing
> the number of system calls substantally.
> 
> Using a strace of test-xenstore as an example, we go from this:
> 
>   rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, 8) = 0
>   write(3, "\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", 16) = 16
>   write(3, "xenstore-test/502673/a\0", 23) = 23
>   write(3, "a", 1)                        = 1
>   read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16
>   read(3, "OK\0", 3)                      = 3
>   rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, NULL, 8) = 0
> 
> down to this:
> 
>   sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", iov_len=16}, {iov_base="xenstore-test/504021/a\0", iov_len=23}, {iov_base="a", iov_len=1}], msg_iovlen=3, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 40
>   read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16
>   read(3, "OK\0", 3)                      = 3
> 
> 
> I.e., it removes 2x rt_sigaction(), and turns all write()'s into a single
> writev() or sendmsg().
> 
> 
> Reads are a little more problematic to deal with.  Xenstored will produce a
> full package basically in one go, but libxenstore's reading is horrbly
> complicated by virtue of it being completely different depending on whether
> xs_watch() has created a secondary read thread or not.
> 
> Andrew Cooper (6):
>   tools/libxs: Fix length check in xs_talkv()
>   tools/libxs: Rework xs_talkv() to take xsd_sockmsg within the iovec
>   tools/libxs: Rationalise the definition of struct xs_handle
>   tools/libxs: Track whether we're using a socket or file
>   tools/libxs: Use writev()/sendmsg() instead of write()
>   tools/libxs: Stop playing with SIGPIPE

The title of the entire series containing "Fix" vs there being no single
Fixes: tag throughout, afaics, leave unclear to me whether any or all of
this work wants/needs backporting. Please clarify.

Jan

Re: [PATCH 0/6] tools/libxs: Fix SIGPIPE handling issues

Posted by Andrew Cooper 1 month, 2 weeks ago

On 24/07/2024 7:58 am, Jan Beulich wrote:
> On 18.07.2024 18:48, Andrew Cooper wrote:
>> While the purpose of this series is patch 6, it has a side effect of reducing
>> the number of system calls substantally.
>>
>> Using a strace of test-xenstore as an example, we go from this:
>>
>>   rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, 8) = 0
>>   write(3, "\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", 16) = 16
>>   write(3, "xenstore-test/502673/a\0", 23) = 23
>>   write(3, "a", 1)                        = 1
>>   read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16
>>   read(3, "OK\0", 3)                      = 3
>>   rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, NULL, 8) = 0
>>
>> down to this:
>>
>>   sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", iov_len=16}, {iov_base="xenstore-test/504021/a\0", iov_len=23}, {iov_base="a", iov_len=1}], msg_iovlen=3, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 40
>>   read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16
>>   read(3, "OK\0", 3)                      = 3
>>
>>
>> I.e., it removes 2x rt_sigaction(), and turns all write()'s into a single
>> writev() or sendmsg().
>>
>>
>> Reads are a little more problematic to deal with.  Xenstored will produce a
>> full package basically in one go, but libxenstore's reading is horrbly
>> complicated by virtue of it being completely different depending on whether
>> xs_watch() has created a secondary read thread or not.
>>
>> Andrew Cooper (6):
>>   tools/libxs: Fix length check in xs_talkv()
>>   tools/libxs: Rework xs_talkv() to take xsd_sockmsg within the iovec
>>   tools/libxs: Rationalise the definition of struct xs_handle
>>   tools/libxs: Track whether we're using a socket or file
>>   tools/libxs: Use writev()/sendmsg() instead of write()
>>   tools/libxs: Stop playing with SIGPIPE
> The title of the entire series containing "Fix" vs there being no single
> Fixes: tag throughout, afaics, leave unclear to me whether any or all of
> this work wants/needs backporting. Please clarify.

I was on the fence about whether I should have used Fixes: (2005 commit)
in patch 6 or not.

Also I've had a spectacularly bad run of luck with the CLOEXEC fixes so
leaving them to bake for a little longer in staging is no bad thing. 
That said, I am likely to backport them into the patchqueue imminently.

Personally, I think it is very buggy for libxenstore to play with
SIGPIPE, but it also came from reading the strace of the CLOEXEC fixes
and thinking "well that's clearly broken too" rather than from a real
SIGPIPE problem.

~Andrew

P.S. Then again, the Xen libraries all get away with murder doing things
that libraries must never do...  I think it's still unforgivable that
libxentoollog adjusts stdout/stderr if you pass NULL into
xc_open_handle(), and that we have 5 different trivial few-kb libraries
which are co-dependent on each other when it should be a single one.