diff mbox series

[RFC,v2,3/6] vsock/vmci: always return ENOMEM in case of error

Message ID 675b1f93-dc07-0a70-0622-c3fc6236c8bb@sberdevices.ru
State New
Headers
Series vsock: update tools and error handling |

Commit Message

Arseniy Krasnov Nov. 25, 2022, 5:08 p.m. UTC 
  From: Bobby Eshleman <bobby.eshleman@bytedance.com>

This saves original behaviour from af_vsock.c - switch any error
code returned from transport layer to ENOMEM.

Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 net/vmw_vsock/vmci_transport.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.25.1

Comments

Arseniy Krasnov Dec. 1, 2022, 11:36 a.m. UTC | #1 
On 01.12.2022 12:30, Stefano Garzarella wrote:
> On Fri, Nov 25, 2022 at 05:08:06PM +0000, Arseniy Krasnov wrote:
>> From: Bobby Eshleman <bobby.eshleman@bytedance.com>
>>
>> This saves original behaviour from af_vsock.c - switch any error
>> code returned from transport layer to ENOMEM.
>>
>> Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> net/vmw_vsock/vmci_transport.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> @Bryan @Vishnu what do you think about this patch?
> 
> A bit of context:
> 
> Before this series, the af_vsock core always returned ENOMEM to the user if the transport failed to queue the packet.
> 
> Now we are changing it by returning the transport error. So I think here we want to preserve the previous behavior for vmci, but I don't know if that's the right thing.
> 
> 
> 
> @Arseniy please in the next versions describe better in the commit messages the reasons for these changes, so it is easier review for others and also in the future by reading the commit message we can understand the reason for the change.
Hello,

Sure! Sorry for that! Also, I can send both vmci and hyperv patches in the next version(e.g. not waiting for
reviewers reply and reorder them with 1/6 as You asked), as result of review could be dropped patch only.

Thanks, Arseniy
> 
> Thanks,
> Stefano
> 
>>
>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>> index 842c94286d31..289a36a203a2 100644
>> --- a/net/vmw_vsock/vmci_transport.c
>> +++ b/net/vmw_vsock/vmci_transport.c
>> @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
>>     struct msghdr *msg,
>>     size_t len)
>> {
>> -    return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +    int err;
>> +
>> +    err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +
>> +    if (err < 0)
>> +        err = -ENOMEM;
>> +
>> +    return err;
>> }
>>
>> static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
>> -- 
>> 2.25.1
>
Vishnu Dasa Dec. 1, 2022, 3:14 p.m. UTC | #2 
> On Dec 1, 2022, at 1:30 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> !! External Email
> 
> On Fri, Nov 25, 2022 at 05:08:06PM +0000, Arseniy Krasnov wrote:
>> From: Bobby Eshleman <bobby.eshleman@bytedance.com>
>> 
>> This saves original behaviour from af_vsock.c - switch any error
>> code returned from transport layer to ENOMEM.
>> 
>> Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> net/vmw_vsock/vmci_transport.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> @Bryan @Vishnu what do you think about this patch?
> 
> A bit of context:
> 
> Before this series, the af_vsock core always returned ENOMEM to the user
> if the transport failed to queue the packet.
> 
> Now we are changing it by returning the transport error. So I think here
> we want to preserve the previous behavior for vmci, but I don't know if
> that's the right thing.
> 

Agree with Stefano.  I don't think we need to preserve the previous
behavior for vmci.

> 
> @Arseniy please in the next versions describe better in the commit
> messages the reasons for these changes, so it is easier review for
> others and also in the future by reading the commit message we can
> understand the reason for the change.
> 
> Thanks,
> Stefano
> 
>> 
>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>> index 842c94286d31..289a36a203a2 100644
>> --- a/net/vmw_vsock/vmci_transport.c
>> +++ b/net/vmw_vsock/vmci_transport.c
>> @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
>>      struct msghdr *msg,
>>      size_t len)
>> {
>> -      return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +      int err;
>> +
>> +      err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +
>> +      if (err < 0)
>> +              err = -ENOMEM;
>> +
>> +      return err;
>> }
>> 
>> static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
>> --
>> 2.25.1
> 
> 
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.
Arseniy Krasnov Dec. 1, 2022, 3:16 p.m. UTC | #3 
On 01.12.2022 18:14, Vishnu Dasa wrote:
> 
> 
>> On Dec 1, 2022, at 1:30 AM, Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> !! External Email
>>
>> On Fri, Nov 25, 2022 at 05:08:06PM +0000, Arseniy Krasnov wrote:
>>> From: Bobby Eshleman <bobby.eshleman@bytedance.com>
>>>
>>> This saves original behaviour from af_vsock.c - switch any error
>>> code returned from transport layer to ENOMEM.
>>>
>>> Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>> ---
>>> net/vmw_vsock/vmci_transport.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> @Bryan @Vishnu what do you think about this patch?
>>
>> A bit of context:
>>
>> Before this series, the af_vsock core always returned ENOMEM to the user
>> if the transport failed to queue the packet.
>>
>> Now we are changing it by returning the transport error. So I think here
>> we want to preserve the previous behavior for vmci, but I don't know if
>> that's the right thing.
>>
> 
> Agree with Stefano.  I don't think we need to preserve the previous
> behavior for vmci.
Good! I'll remove this patch from the next version

Thanks, Arseniy
> 
>>
>> @Arseniy please in the next versions describe better in the commit
>> messages the reasons for these changes, so it is easier review for
>> others and also in the future by reading the commit message we can
>> understand the reason for the change.
>>
>> Thanks,
>> Stefano
>>
>>>
>>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>>> index 842c94286d31..289a36a203a2 100644
>>> --- a/net/vmw_vsock/vmci_transport.c
>>> +++ b/net/vmw_vsock/vmci_transport.c
>>> @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
>>>      struct msghdr *msg,
>>>      size_t len)
>>> {
>>> -      return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>>> +      int err;
>>> +
>>> +      err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>>> +
>>> +      if (err < 0)
>>> +              err = -ENOMEM;
>>> +
>>> +      return err;
>>> }
>>>
>>> static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
>>> --
>>> 2.25.1
>>
>>
>> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.
>
diff mbox series

Patch

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 842c94286d31..289a36a203a2 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1838,7 +1838,14 @@  static ssize_t vmci_transport_stream_enqueue(
 	struct msghdr *msg,
 	size_t len)
 {
-	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+	int err;
+
+	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+
+	if (err < 0)
+		err = -ENOMEM;
+
+	return err;
 }
 
 static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)