[Qemu-devel] [PATCH v4] tap: setting error appropriately when calling net_init_tap_one()

[Qemu-devel] [PATCH v4] tap: setting error appropriately when calling net_init_tap_one()

[Jay Zhou]

If netdev_add tap,id=net0,...,vhost=on failed in net_init_tap_one(),
the followed up device_add virtio-net-pci,netdev=net0 will fail
too, prints:

   TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
   ioctl() failed: Bad file descriptor

The reason is that the fd of tap is closed when error occured after
calling net_init_tap_one().

The fd should be closed when calling net_init_tap_one failed:
   - if tap_set_sndbuf() failed
   - if tap_set_sndbuf() succeeded but vhost failed to open or
     initialize with vhostforce flag on
The fd should not be closed just because vhost failed to open or
initialize but without vhostforce flag. So the followed up
device_add can fall back to userspace virtio successfully.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
v4: - reduce duplication
    - close the fd by caller
    - tweak the title

v3: - set errp appropriately
---
 include/net/vhost_net.h |  3 +++
 net/tap.c               | 24 ++++++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index afc1499..77e4739 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -4,6 +4,9 @@
 #include "net/net.h"
 #include "hw/virtio/vhost-backend.h"
 
+#define VHOST_NET_INIT_FAILED \
+    "vhost-net requested but could not be initialized"
+
 struct vhost_net;
 typedef struct vhost_net VHostNetState;
 
diff --git a/net/tap.c b/net/tap.c
index 979e622..14d230f 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -686,14 +686,23 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         if (vhostfdname) {
             vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
             if (vhostfd == -1) {
-                error_propagate(errp, err);
+                if (tap->has_vhostforce && tap->vhostforce) {
+                    error_propagate(errp, err);
+                } else {
+                    warn_report_err(err);
+                }
                 return;
             }
         } else {
             vhostfd = open("/dev/vhost-net", O_RDWR);
             if (vhostfd < 0) {
-                error_setg_errno(errp, errno,
-                                 "tap: open vhost char device failed");
+                if (tap->has_vhostforce && tap->vhostforce) {
+                    error_setg_errno(errp, errno,
+                                     "tap: open vhost char device failed");
+                } else {
+                    warn_report("tap: open vhost char device failed: %s",
+                                strerror(errno));
+                }
                 return;
             }
             fcntl(vhostfd, F_SETFL, O_NONBLOCK);
@@ -702,12 +711,15 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
 
         s->vhost_net = vhost_net_init(&options);
         if (!s->vhost_net) {
-            error_setg(errp,
-                       "vhost-net requested but could not be initialized");
+            if (tap->has_vhostforce && tap->vhostforce) {
+                error_setg(errp, VHOST_NET_INIT_FAILED);
+            } else {
+                warn_report(VHOST_NET_INIT_FAILED);
+            }
             return;
         }
     } else if (vhostfdname) {
-        error_setg(errp, "vhostfd(s)= is not valid without vhost");
+        warn_report("vhostfd(s)= is not valid without vhost");
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v4] tap: setting error appropriately when calling net_init_tap_one()

[Zhoujian (jay)]

Ping ...

> -----Original Message-----
> From: Zhoujian (jay)
> Sent: Tuesday, February 06, 2018 8:54 PM
> To: qemu-devel@nongnu.org
> Cc: jasowang@redhat.com; mst@redhat.com; imammedo@redhat.com; Huangweidong (C)
> <weidong.huang@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei
> (Arei) <arei.gonglei@huawei.com>; Zhoujian (jay) <jianjay.zhou@huawei.com>
> Subject: [PATCH v4] tap: setting error appropriately when calling
> net_init_tap_one()
> 
> If netdev_add tap,id=net0,...,vhost=on failed in net_init_tap_one(), the
> followed up device_add virtio-net-pci,netdev=net0 will fail too, prints:
> 
>    TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
>    ioctl() failed: Bad file descriptor
> 
> The reason is that the fd of tap is closed when error occured after calling
> net_init_tap_one().
> 
> The fd should be closed when calling net_init_tap_one failed:
>    - if tap_set_sndbuf() failed
>    - if tap_set_sndbuf() succeeded but vhost failed to open or
>      initialize with vhostforce flag on
> The fd should not be closed just because vhost failed to open or initialize
> but without vhostforce flag. So the followed up device_add can fall back to
> userspace virtio successfully.
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> ---
> v4: - reduce duplication
>     - close the fd by caller
>     - tweak the title
> 
> v3: - set errp appropriately
> ---
>  include/net/vhost_net.h |  3 +++
>  net/tap.c               | 24 ++++++++++++++++++------
>  2 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h index
> afc1499..77e4739 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -4,6 +4,9 @@
>  #include "net/net.h"
>  #include "hw/virtio/vhost-backend.h"
> 
> +#define VHOST_NET_INIT_FAILED \
> +    "vhost-net requested but could not be initialized"
> +
>  struct vhost_net;
>  typedef struct vhost_net VHostNetState;
> 
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..14d230f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -686,14 +686,23 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
>          if (vhostfdname) {
>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>              if (vhostfd == -1) {
> -                error_propagate(errp, err);
> +                if (tap->has_vhostforce && tap->vhostforce) {
> +                    error_propagate(errp, err);
> +                } else {
> +                    warn_report_err(err);
> +                }
>                  return;
>              }
>          } else {
>              vhostfd = open("/dev/vhost-net", O_RDWR);
>              if (vhostfd < 0) {
> -                error_setg_errno(errp, errno,
> -                                 "tap: open vhost char device failed");
> +                if (tap->has_vhostforce && tap->vhostforce) {
> +                    error_setg_errno(errp, errno,
> +                                     "tap: open vhost char device failed");
> +                } else {
> +                    warn_report("tap: open vhost char device failed: %s",
> +                                strerror(errno));
> +                }
>                  return;
>              }
>              fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -702,12 +711,15 @@
> static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState
> *peer,
> 
>          s->vhost_net = vhost_net_init(&options);
>          if (!s->vhost_net) {
> -            error_setg(errp,
> -                       "vhost-net requested but could not be initialized");
> +            if (tap->has_vhostforce && tap->vhostforce) {
> +                error_setg(errp, VHOST_NET_INIT_FAILED);
> +            } else {
> +                warn_report(VHOST_NET_INIT_FAILED);
> +            }
>              return;
>          }
>      } else if (vhostfdname) {
> -        error_setg(errp, "vhostfd(s)= is not valid without vhost");
> +        warn_report("vhostfd(s)= is not valid without vhost");
>      }
>  }
> 
> --
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v4] tap: setting error appropriately when calling net_init_tap_one()

[Jason Wang]

On 2018年02月06日 20:53, Jay Zhou wrote:
> If netdev_add tap,id=net0,...,vhost=on failed in net_init_tap_one(),
> the followed up device_add virtio-net-pci,netdev=net0 will fail
> too, prints:
>
>     TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
>     ioctl() failed: Bad file descriptor
>
> The reason is that the fd of tap is closed when error occured after
> calling net_init_tap_one().
>
> The fd should be closed when calling net_init_tap_one failed:
>     - if tap_set_sndbuf() failed
>     - if tap_set_sndbuf() succeeded but vhost failed to open or
>       initialize with vhostforce flag on
> The fd should not be closed just because vhost failed to open or
> initialize but without vhostforce flag. So the followed up
> device_add can fall back to userspace virtio successfully.
>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> ---
> v4: - reduce duplication
>      - close the fd by caller
>      - tweak the title
>
> v3: - set errp appropriately
> ---
>   include/net/vhost_net.h |  3 +++
>   net/tap.c               | 24 ++++++++++++++++++------
>   2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index afc1499..77e4739 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -4,6 +4,9 @@
>   #include "net/net.h"
>   #include "hw/virtio/vhost-backend.h"
>   
> +#define VHOST_NET_INIT_FAILED \
> +    "vhost-net requested but could not be initialized"
> +
>   struct vhost_net;
>   typedef struct vhost_net VHostNetState;
>   
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..14d230f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -686,14 +686,23 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>           if (vhostfdname) {
>               vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>               if (vhostfd == -1) {
> -                error_propagate(errp, err);
> +                if (tap->has_vhostforce && tap->vhostforce) {
> +                    error_propagate(errp, err);
> +                } else {
> +                    warn_report_err(err);
> +                }
>                   return;
>               }
>           } else {
>               vhostfd = open("/dev/vhost-net", O_RDWR);
>               if (vhostfd < 0) {
> -                error_setg_errno(errp, errno,
> -                                 "tap: open vhost char device failed");
> +                if (tap->has_vhostforce && tap->vhostforce) {
> +                    error_setg_errno(errp, errno,
> +                                     "tap: open vhost char device failed");
> +                } else {
> +                    warn_report("tap: open vhost char device failed: %s",
> +                                strerror(errno));
> +                }
>                   return;
>               }
>               fcntl(vhostfd, F_SETFL, O_NONBLOCK);
> @@ -702,12 +711,15 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>   
>           s->vhost_net = vhost_net_init(&options);
>           if (!s->vhost_net) {
> -            error_setg(errp,
> -                       "vhost-net requested but could not be initialized");
> +            if (tap->has_vhostforce && tap->vhostforce) {
> +                error_setg(errp, VHOST_NET_INIT_FAILED);
> +            } else {
> +                warn_report(VHOST_NET_INIT_FAILED);
> +            }
>               return;
>           }
>       } else if (vhostfdname) {
> -        error_setg(errp, "vhostfd(s)= is not valid without vhost");
> +        warn_report("vhostfd(s)= is not valid without vhost");

Do we need to keep the error here consider it was a wrong command line 
parameter?

Thanks

>       }
>   }
>   

Re: [Qemu-devel] [PATCH v4] tap: setting error appropriately when calling net_init_tap_one()

[Zhoujian (jay)]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Friday, March 02, 2018 2:30 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; Gonglei
> (Arei) <arei.gonglei@huawei.com>; imammedo@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v4] tap: setting error appropriately when
> calling net_init_tap_one()
> 
> 
> 
> On 2018年02月06日 20:53, Jay Zhou wrote:
> > If netdev_add tap,id=net0,...,vhost=on failed in net_init_tap_one(),
> > the followed up device_add virtio-net-pci,netdev=net0 will fail too,
> > prints:
> >
> >     TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
> >     ioctl() failed: Bad file descriptor
> >
> > The reason is that the fd of tap is closed when error occured after
> > calling net_init_tap_one().
> >
> > The fd should be closed when calling net_init_tap_one failed:
> >     - if tap_set_sndbuf() failed
> >     - if tap_set_sndbuf() succeeded but vhost failed to open or
> >       initialize with vhostforce flag on The fd should not be closed
> > just because vhost failed to open or initialize but without vhostforce
> > flag. So the followed up device_add can fall back to userspace virtio
> > successfully.
> >
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> > ---
> > v4: - reduce duplication
> >      - close the fd by caller
> >      - tweak the title
> >
> > v3: - set errp appropriately
> > ---
> >   include/net/vhost_net.h |  3 +++
> >   net/tap.c               | 24 ++++++++++++++++++------
> >   2 files changed, 21 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h index
> > afc1499..77e4739 100644
> > --- a/include/net/vhost_net.h
> > +++ b/include/net/vhost_net.h
> > @@ -4,6 +4,9 @@
> >   #include "net/net.h"
> >   #include "hw/virtio/vhost-backend.h"
> >
> > +#define VHOST_NET_INIT_FAILED \
> > +    "vhost-net requested but could not be initialized"
> > +
> >   struct vhost_net;
> >   typedef struct vhost_net VHostNetState;
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index 979e622..14d230f 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -686,14 +686,23 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> >           if (vhostfdname) {
> >               vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> >               if (vhostfd == -1) {
> > -                error_propagate(errp, err);
> > +                if (tap->has_vhostforce && tap->vhostforce) {
> > +                    error_propagate(errp, err);
> > +                } else {
> > +                    warn_report_err(err);
> > +                }
> >                   return;
> >               }
> >           } else {
> >               vhostfd = open("/dev/vhost-net", O_RDWR);
> >               if (vhostfd < 0) {
> > -                error_setg_errno(errp, errno,
> > -                                 "tap: open vhost char device failed");
> > +                if (tap->has_vhostforce && tap->vhostforce) {
> > +                    error_setg_errno(errp, errno,
> > +                                     "tap: open vhost char device failed");
> > +                } else {
> > +                    warn_report("tap: open vhost char device failed: %s",
> > +                                strerror(errno));
> > +                }
> >                   return;
> >               }
> >               fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -702,12 +711,15
> > @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> > NetClientState *peer,
> >
> >           s->vhost_net = vhost_net_init(&options);
> >           if (!s->vhost_net) {
> > -            error_setg(errp,
> > -                       "vhost-net requested but could not be initialized");
> > +            if (tap->has_vhostforce && tap->vhostforce) {
> > +                error_setg(errp, VHOST_NET_INIT_FAILED);
> > +            } else {
> > +                warn_report(VHOST_NET_INIT_FAILED);
> > +            }
> >               return;
> >           }
> >       } else if (vhostfdname) {
> > -        error_setg(errp, "vhostfd(s)= is not valid without vhost");
> > +        warn_report("vhostfd(s)= is not valid without vhost");
> 
> Do we need to keep the error here consider it was a wrong command line
> parameter?

Fair enough, thanks!

Regards,
Jay

> 
> Thanks
> 
> >       }
> >   }
> >