tools/libxs: Fix SIGPIPE handling issues

[PATCH 4/6] tools/libxs: Track whether we're using a socket or file

Posted by Andrew Cooper 1 year, 6 months ago

It will determine whether to use writev() or sendmsg().

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 96ea2b192924..f4edeb05f03b 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -65,6 +65,9 @@ struct xs_stored_msg {
 struct xs_handle {
 	/* Communications channel to xenstore daemon. */
 	int fd;
+
+	bool is_socket; /* is @fd a file or socket? */
+
 	Xentoolcore__Active_Handle tc_ah; /* for restrict */
 
 	/*
@@ -305,6 +308,8 @@ static struct xs_handle *get_handle(const char *connect_to)
 	if (h->fd == -1)
 		goto err;
 
+	h->is_socket = S_ISSOCK(buf.st_mode);
+
 	XEN_TAILQ_INIT(&h->reply_list);
 	XEN_TAILQ_INIT(&h->watch_list);
 
-- 
2.39.2

Re: [PATCH 4/6] tools/libxs: Track whether we're using a socket or file

Posted by Jürgen Groß 1 year, 6 months ago

On 18.07.24 18:48, Andrew Cooper wrote:
> It will determine whether to use writev() or sendmsg().
> 
> 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 | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 96ea2b192924..f4edeb05f03b 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -65,6 +65,9 @@ struct xs_stored_msg {
>   struct xs_handle {
>   	/* Communications channel to xenstore daemon. */
>   	int fd;
> +
> +	bool is_socket; /* is @fd a file or socket? */
> +
>   	Xentoolcore__Active_Handle tc_ah; /* for restrict */
>   
>   	/*
> @@ -305,6 +308,8 @@ static struct xs_handle *get_handle(const char *connect_to)
>   	if (h->fd == -1)
>   		goto err;
>   
> +	h->is_socket = S_ISSOCK(buf.st_mode);
> +
>   	XEN_TAILQ_INIT(&h->reply_list);
>   	XEN_TAILQ_INIT(&h->watch_list);
>   

I'd prefer to set h->is_socket above the current

     if (S_ISSOCK(buf.st_mode))

and then use h->is_socket in this if instead.

With that:

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


Juergen

Re: [PATCH 4/6] tools/libxs: Track whether we're using a socket or file

Posted by Andrew Cooper 1 year, 6 months ago

On 23/07/2024 10:33 am, Jürgen Groß wrote:
> On 18.07.24 18:48, Andrew Cooper wrote:
>> It will determine whether to use writev() or sendmsg().
>>
>> 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 | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>> index 96ea2b192924..f4edeb05f03b 100644
>> --- a/tools/libs/store/xs.c
>> +++ b/tools/libs/store/xs.c
>> @@ -65,6 +65,9 @@ struct xs_stored_msg {
>>   struct xs_handle {
>>       /* Communications channel to xenstore daemon. */
>>       int fd;
>> +
>> +    bool is_socket; /* is @fd a file or socket? */
>> +
>>       Xentoolcore__Active_Handle tc_ah; /* for restrict */
>>         /*
>> @@ -305,6 +308,8 @@ static struct xs_handle *get_handle(const char
>> *connect_to)
>>       if (h->fd == -1)
>>           goto err;
>>   +    h->is_socket = S_ISSOCK(buf.st_mode);
>> +
>>       XEN_TAILQ_INIT(&h->reply_list);
>>       XEN_TAILQ_INIT(&h->watch_list);
>>   
>
> I'd prefer to set h->is_socket above the current
>
>     if (S_ISSOCK(buf.st_mode))
>
> and then use h->is_socket in this if instead.
>
> With that:
>
> Reviewed-by: Juergen Gross <jgross@suse.com>

To confirm, you mean like this?

@@ -297,7 +300,9 @@ static struct xs_handle *get_handle(const char
*connect_to)
        if (stat(connect_to, &buf) != 0)
                goto err;
 
-       if (S_ISSOCK(buf.st_mode))
+       h->is_socket = S_ISSOCK(buf.st_mode);
+
+       if (h->is_socket)
                h->fd = get_socket(connect_to);
        else
                h->fd = get_dev(connect_to);

Re: [PATCH 4/6] tools/libxs: Track whether we're using a socket or file

Posted by Jürgen Groß 1 year, 6 months ago

On 23.07.24 14:07, Andrew Cooper wrote:
> On 23/07/2024 10:33 am, Jürgen Groß wrote:
>> On 18.07.24 18:48, Andrew Cooper wrote:
>>> It will determine whether to use writev() or sendmsg().
>>>
>>> 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 | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>> index 96ea2b192924..f4edeb05f03b 100644
>>> --- a/tools/libs/store/xs.c
>>> +++ b/tools/libs/store/xs.c
>>> @@ -65,6 +65,9 @@ struct xs_stored_msg {
>>>    struct xs_handle {
>>>        /* Communications channel to xenstore daemon. */
>>>        int fd;
>>> +
>>> +    bool is_socket; /* is @fd a file or socket? */
>>> +
>>>        Xentoolcore__Active_Handle tc_ah; /* for restrict */
>>>          /*
>>> @@ -305,6 +308,8 @@ static struct xs_handle *get_handle(const char
>>> *connect_to)
>>>        if (h->fd == -1)
>>>            goto err;
>>>    +    h->is_socket = S_ISSOCK(buf.st_mode);
>>> +
>>>        XEN_TAILQ_INIT(&h->reply_list);
>>>        XEN_TAILQ_INIT(&h->watch_list);
>>>    
>>
>> I'd prefer to set h->is_socket above the current
>>
>>      if (S_ISSOCK(buf.st_mode))
>>
>> and then use h->is_socket in this if instead.
>>
>> With that:
>>
>> Reviewed-by: Juergen Gross <jgross@suse.com>
> 
> To confirm, you mean like this?
> 
> @@ -297,7 +300,9 @@ static struct xs_handle *get_handle(const char
> *connect_to)
>          if (stat(connect_to, &buf) != 0)
>                  goto err;
>   
> -       if (S_ISSOCK(buf.st_mode))
> +       h->is_socket = S_ISSOCK(buf.st_mode);
> +
> +       if (h->is_socket)
>                  h->fd = get_socket(connect_to);
>          else
>                  h->fd = get_dev(connect_to);
> 

Yes.


Juergen

Re: [PATCH 4/6] tools/libxs: Track whether we're using a socket or file

Posted by Jason Andryuk 1 year, 6 months ago

On 2024-07-23 08:08, Jürgen Groß wrote:
> On 23.07.24 14:07, Andrew Cooper wrote:
>> On 23/07/2024 10:33 am, Jürgen Groß wrote:
>>> On 18.07.24 18:48, Andrew Cooper wrote:
>>>> It will determine whether to use writev() or sendmsg().
>>>>
>>>> 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 | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
>>>> index 96ea2b192924..f4edeb05f03b 100644
>>>> --- a/tools/libs/store/xs.c
>>>> +++ b/tools/libs/store/xs.c
>>>> @@ -65,6 +65,9 @@ struct xs_stored_msg {
>>>>    struct xs_handle {
>>>>        /* Communications channel to xenstore daemon. */
>>>>        int fd;
>>>> +
>>>> +    bool is_socket; /* is @fd a file or socket? */
>>>> +
>>>>        Xentoolcore__Active_Handle tc_ah; /* for restrict */
>>>>          /*
>>>> @@ -305,6 +308,8 @@ static struct xs_handle *get_handle(const char
>>>> *connect_to)
>>>>        if (h->fd == -1)
>>>>            goto err;
>>>>    +    h->is_socket = S_ISSOCK(buf.st_mode);
>>>> +
>>>>        XEN_TAILQ_INIT(&h->reply_list);
>>>>        XEN_TAILQ_INIT(&h->watch_list);
>>>
>>> I'd prefer to set h->is_socket above the current
>>>
>>>      if (S_ISSOCK(buf.st_mode))
>>>
>>> and then use h->is_socket in this if instead.
>>>
>>> With that:
>>>
>>> Reviewed-by: Juergen Gross <jgross@suse.com>
>>
>> To confirm, you mean like this?
>>
>> @@ -297,7 +300,9 @@ static struct xs_handle *get_handle(const char
>> *connect_to)
>>          if (stat(connect_to, &buf) != 0)
>>                  goto err;
>> -       if (S_ISSOCK(buf.st_mode))
>> +       h->is_socket = S_ISSOCK(buf.st_mode);
>> +
>> +       if (h->is_socket)
>>                  h->fd = get_socket(connect_to);
>>          else
>>                  h->fd = get_dev(connect_to);
>>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>