diff mbox series

virtio-user: fix backend selection if stat fails

Message ID 20201020071628.323641-1-amorenoz@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series virtio-user: fix backend selection if stat fails |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing success Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

Adrian Moreno Oct. 20, 2020, 7:16 a.m. UTC 
  If stat fails it means the backend must be vhost-user in server mode

Bugzilla ID: 559
Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Yu Jiang Oct. 20, 2020, 8:43 a.m. UTC | #1 
Tested-by: JiangYuX <yux.jiang@intel.com>

    Best Regards
    Jiang yu


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrian Moreno
> Sent: Tuesday, October 20, 2020 3:16 PM
> To: dev@dpdk.org
> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
> <patrick.fu@intel.com>; amorenoz@redhat.com; stable@dpdk.org; Maxime
> Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
> Subject: [dpdk-dev] [PATCH] virtio-user: fix backend selection if stat fails
> 
> If stat fails it means the backend must be vhost-user in server mode
> 
> Bugzilla ID: 559
> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 042665bc0..ce74d08ab 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -560,9 +560,10 @@ virtio_user_backend_type(const char *path)
>  	struct stat sb;
> 
>  	if (stat(path, &sb) == -1) {
> -		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
> +		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
>  			     strerror(errno));
> -		return VIRTIO_USER_BACKEND_UNKNOWN;
> +		/* Must be vhost-user in server mode */
> +		return VIRTIO_USER_BACKEND_VHOST_USER;
>  	}
> 
>  	if (S_ISSOCK(sb.st_mode)) {
> --
> 2.26.2
Kevin Traynor Oct. 20, 2020, 9:01 a.m. UTC | #2 
On 20/10/2020 08:16, Adrian Moreno wrote:
> If stat fails it means the backend must be vhost-user in server mode
> 
> Bugzilla ID: 559
> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index 042665bc0..ce74d08ab 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -560,9 +560,10 @@ virtio_user_backend_type(const char *path)
>  	struct stat sb;
>  
>  	if (stat(path, &sb) == -1) {
> -		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
> +		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
>  			     strerror(errno));

It may be accurate, but a 'fail' in the logs can be confusing for users
when it is an INFO log and normal operation. Suggest to reword to
something softer like 'Unable to stat' or 'Not able to get file status'

> -		return VIRTIO_USER_BACKEND_UNKNOWN;
> +		/* Must be vhost-user in server mode */
> +		return VIRTIO_USER_BACKEND_VHOST_USER;
>  	}
>  
>  	if (S_ISSOCK(sb.st_mode)) {
>
Maxime Coquelin Oct. 20, 2020, 9:11 a.m. UTC | #3 
On 10/20/20 11:01 AM, Kevin Traynor wrote:
> On 20/10/2020 08:16, Adrian Moreno wrote:
>> If stat fails it means the backend must be vhost-user in server mode
>>
>> Bugzilla ID: 559
>> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
>> index 042665bc0..ce74d08ab 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -560,9 +560,10 @@ virtio_user_backend_type(const char *path)
>>  	struct stat sb;
>>  
>>  	if (stat(path, &sb) == -1) {
>> -		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
>> +		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
>>  			     strerror(errno));
> 
> It may be accurate, but a 'fail' in the logs can be confusing for users
> when it is an INFO log and normal operation. Suggest to reword to
> something softer like 'Unable to stat' or 'Not able to get file status'


We may want to:
- only return VIRTIO_USER_BACKEND_VHOST_USER if -ENOENT, and log that
we assume this is Vhost-user backend in server mode at INFO level.
- return VIRTIO_USER_BACKEND_UNKNOWN otherwise and print an error
message with the strerror(errno).

What do you think?

>> -		return VIRTIO_USER_BACKEND_UNKNOWN;
>> +		/* Must be vhost-user in server mode */
>> +		return VIRTIO_USER_BACKEND_VHOST_USER;
>>  	}
>>  
>>  	if (S_ISSOCK(sb.st_mode)) {
>>
>
Kevin Traynor Oct. 20, 2020, 9:38 a.m. UTC | #4 
On 20/10/2020 10:11, Maxime Coquelin wrote:
> 
> 
> On 10/20/20 11:01 AM, Kevin Traynor wrote:
>> On 20/10/2020 08:16, Adrian Moreno wrote:
>>> If stat fails it means the backend must be vhost-user in server mode
>>>
>>> Bugzilla ID: 559
>>> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>  drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
>>> index 042665bc0..ce74d08ab 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -560,9 +560,10 @@ virtio_user_backend_type(const char *path)
>>>  	struct stat sb;
>>>  
>>>  	if (stat(path, &sb) == -1) {
>>> -		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
>>> +		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
>>>  			     strerror(errno));
>>
>> It may be accurate, but a 'fail' in the logs can be confusing for users
>> when it is an INFO log and normal operation. Suggest to reword to
>> something softer like 'Unable to stat' or 'Not able to get file status'
> 
> 
> We may want to:
> - only return VIRTIO_USER_BACKEND_VHOST_USER if -ENOENT, and log that
> we assume this is Vhost-user backend in server mode at INFO level.

It will mean that sometimes the backend type is logged and sometimes
not, but maybe you make a distinction because there is an assumption
being made in this case?

> - return VIRTIO_USER_BACKEND_UNKNOWN otherwise and print an error
> message with the strerror(errno).
> 

yes, it seems better like that.

> What do you think?
> 
>>> -		return VIRTIO_USER_BACKEND_UNKNOWN;
>>> +		/* Must be vhost-user in server mode */
>>> +		return VIRTIO_USER_BACKEND_VHOST_USER;
>>>  	}
>>>  
>>>  	if (S_ISSOCK(sb.st_mode)) {
>>>
>>
Maxime Coquelin Oct. 20, 2020, 9:55 a.m. UTC | #5 
On 10/20/20 11:38 AM, Kevin Traynor wrote:
> On 20/10/2020 10:11, Maxime Coquelin wrote:
>>
>>
>> On 10/20/20 11:01 AM, Kevin Traynor wrote:
>>> On 20/10/2020 08:16, Adrian Moreno wrote:
>>>> If stat fails it means the backend must be vhost-user in server mode
>>>>
>>>> Bugzilla ID: 559
>>>> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>> ---
>>>>  drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
>>>> index 042665bc0..ce74d08ab 100644
>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>> @@ -560,9 +560,10 @@ virtio_user_backend_type(const char *path)
>>>>  	struct stat sb;
>>>>  
>>>>  	if (stat(path, &sb) == -1) {
>>>> -		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
>>>> +		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
>>>>  			     strerror(errno));
>>>
>>> It may be accurate, but a 'fail' in the logs can be confusing for users
>>> when it is an INFO log and normal operation. Suggest to reword to
>>> something softer like 'Unable to stat' or 'Not able to get file status'
>>
>>
>> We may want to:
>> - only return VIRTIO_USER_BACKEND_VHOST_USER if -ENOENT, and log that
>> we assume this is Vhost-user backend in server mode at INFO level.
> 
> It will mean that sometimes the backend type is logged and sometimes
> not, but maybe you make a distinction because there is an assumption
> being made in this case?

I agree it would  make sense to log at INFO level for all backend types.

>> - return VIRTIO_USER_BACKEND_UNKNOWN otherwise and print an error
>> message with the strerror(errno).
>>
> 
> yes, it seems better like that.
> 
>> What do you think?
>>
>>>> -		return VIRTIO_USER_BACKEND_UNKNOWN;
>>>> +		/* Must be vhost-user in server mode */
>>>> +		return VIRTIO_USER_BACKEND_VHOST_USER;
>>>>  	}
>>>>  
>>>>  	if (S_ISSOCK(sb.st_mode)) {
>>>>
>>>
>
Adrian Moreno Oct. 20, 2020, 10:33 a.m. UTC | #6 
On 10/20/20 11:55 AM, Maxime Coquelin wrote:
> 
> 
> On 10/20/20 11:38 AM, Kevin Traynor wrote:
>> On 20/10/2020 10:11, Maxime Coquelin wrote:
>>>
>>>
>>> On 10/20/20 11:01 AM, Kevin Traynor wrote:
>>>> On 20/10/2020 08:16, Adrian Moreno wrote:
>>>>> If stat fails it means the backend must be vhost-user in server mode
>>>>>
>>>>> Bugzilla ID: 559
>>>>> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>>> ---
>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 5 +++--
>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
>>>>> index 042665bc0..ce74d08ab 100644
>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>> @@ -560,9 +560,10 @@ virtio_user_backend_type(const char *path)
>>>>>  	struct stat sb;
>>>>>  
>>>>>  	if (stat(path, &sb) == -1) {
>>>>> -		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
>>>>> +		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
>>>>>  			     strerror(errno));
>>>>
>>>> It may be accurate, but a 'fail' in the logs can be confusing for users
>>>> when it is an INFO log and normal operation. Suggest to reword to
>>>> something softer like 'Unable to stat' or 'Not able to get file status'
>>>
>>>
>>> We may want to:
>>> - only return VIRTIO_USER_BACKEND_VHOST_USER if -ENOENT, and log that
>>> we assume this is Vhost-user backend in server mode at INFO level.
>>
>> It will mean that sometimes the backend type is logged and sometimes
>> not, but maybe you make a distinction because there is an assumption
>> being made in this case?
> 
> I agree it would  make sense to log at INFO level for all backend types.
> 
>>> - return VIRTIO_USER_BACKEND_UNKNOWN otherwise and print an error
>>> message with the strerror(errno).
>>>
>>
>> yes, it seems better like that.
>>
Thanks Maxime and Kevin for your feedback and Jiang for testing. I'll send a v2
addressing the comments.


>>> What do you think?
>>>
>>>>> -		return VIRTIO_USER_BACKEND_UNKNOWN;
>>>>> +		/* Must be vhost-user in server mode */
>>>>> +		return VIRTIO_USER_BACKEND_VHOST_USER;
>>>>>  	}
>>>>>  
>>>>>  	if (S_ISSOCK(sb.st_mode)) {
>>>>>
>>>>
>>
>
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 042665bc0..ce74d08ab 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -560,9 +560,10 @@  virtio_user_backend_type(const char *path)
 	struct stat sb;
 
 	if (stat(path, &sb) == -1) {
-		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
+		PMD_INIT_LOG(INFO, "Stat fails: %s (%s)\n", path,
 			     strerror(errno));
-		return VIRTIO_USER_BACKEND_UNKNOWN;
+		/* Must be vhost-user in server mode */
+		return VIRTIO_USER_BACKEND_VHOST_USER;
 	}
 
 	if (S_ISSOCK(sb.st_mode)) {