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
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
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)) { >
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)) { >> >
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)) { >>> >>
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)) { >>>> >>> >
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)) { >>>>> >>>> >> > -- Adrián Moreno