[Qemu-devel] [PATCH v2 0/2] Add --disable-vhost-user

[Qemu-devel] [PATCH v2 0/2] Add --disable-vhost-user

[Marc-André Lureau]

Hi,

The series aims to add --disable-vhost-user configure option, to
disable all vhost-user support.

v2:
- remove some #ifdef, reuse CONFIG_VHOST_NET_USED
- split the patch to have net/vhost-user.c compiled out (adding 2 ifdefs)
- fail if --enable-vhost-user on win32

Marc-André Lureau (2):
  build-sys: add --disable-vhost-user
  build-sys: do not compile net/vhost-user.c if vhost-user is disabled

 hw/net/vhost_net.c                |  4 ++++
 hw/virtio/virtio-pci.c            |  4 ++--
 configure                         | 29 +++++++++++++++++++++++++++--
 default-configs/pci.mak           |  2 +-
 default-configs/s390x-softmmu.mak |  2 +-
 net/Makefile.objs                 |  2 +-
 tests/Makefile.include            |  6 +++---
 7 files changed, 39 insertions(+), 10 deletions(-)

-- 
2.14.0.rc0.1.g40ca67566

[Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user

[Marc-André Lureau]

Learn to compile out vhost-user. Keep it enabled by default on
non-win32, that is assumed to be POSIX. Fail if trying to enable it on
win32.

When trying to make a vhost-user netdev, it gives the following error:

-netdev vhost-user,id=foo,chardev=chr-test: Parameter 'type' expects a netdev backend type

And similar error with the HMP/QMP monitors.

While at it, rename CONFIG_VHOST_NET_TEST CONFIG_VHOST_USER_NET_TEST
since it's a vhost-user specific variable.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/virtio-pci.c            |  4 ++--
 configure                         | 29 +++++++++++++++++++++++++++--
 default-configs/pci.mak           |  2 +-
 default-configs/s390x-softmmu.mak |  2 +-
 tests/Makefile.include            |  6 +++---
 5 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 5d14bd66dc..8b0d6b69cd 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2135,7 +2135,7 @@ static const TypeInfo vhost_scsi_pci_info = {
 };
 #endif
 
-#ifdef CONFIG_LINUX
+#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
 /* vhost-user-scsi-pci */
 static Property vhost_user_scsi_pci_properties[] = {
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
@@ -2665,7 +2665,7 @@ static void virtio_pci_register_types(void)
 #ifdef CONFIG_VHOST_SCSI
     type_register_static(&vhost_scsi_pci_info);
 #endif
-#ifdef CONFIG_LINUX
+#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
     type_register_static(&vhost_user_scsi_pci_info);
 #endif
 #ifdef CONFIG_VHOST_VSOCK
diff --git a/configure b/configure
index 987f59ba88..efec1a613e 100755
--- a/configure
+++ b/configure
@@ -306,6 +306,7 @@ tcg="yes"
 vhost_net="no"
 vhost_scsi="no"
 vhost_vsock="no"
+vhost_user=""
 kvm="no"
 hax="no"
 rdma=""
@@ -1282,6 +1283,15 @@ for opt do
   ;;
   --enable-vxhs) vxhs="yes"
   ;;
+  --disable-vhost-user) vhost_user="no"
+  ;;
+  --enable-vhost-user)
+      vhost_user="yes"
+      if test "$mingw32" = "yes" ; then
+          echo "ERROR: vhost-user isn't available on win32"
+          exit 1
+      fi
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1290,6 +1300,14 @@ for opt do
   esac
 done
 
+if test "$vhost_user" = ""; then
+    if test "$mingw32" = "yes" ; then
+        vhost_user="no"
+    else
+        vhost_user="yes"
+    fi
+fi
+
 case "$cpu" in
     ppc)
            CPU_CFLAGS="-m32"
@@ -1518,6 +1536,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   tools           build qemu-io, qemu-nbd and qemu-image tools
   vxhs            Veritas HyperScale vDisk backend support
   crypto-afalg    Linux AF_ALG crypto backend driver
+  vhost-user      vhost-user support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -5348,6 +5367,7 @@ echo "libcap-ng support $cap_ng"
 echo "vhost-net support $vhost_net"
 echo "vhost-scsi support $vhost_scsi"
 echo "vhost-vsock support $vhost_vsock"
+echo "vhost-user support $vhost_user"
 echo "Trace backends    $trace_backends"
 if have_backend "simple"; then
 echo "Trace output file $trace_file-<pid>"
@@ -5757,12 +5777,15 @@ fi
 if test "$vhost_scsi" = "yes" ; then
   echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
 fi
-if test "$vhost_net" = "yes" ; then
+if test "$vhost_net" = "yes" -a "$vhost_user" = "yes"; then
   echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
 fi
 if test "$vhost_vsock" = "yes" ; then
   echo "CONFIG_VHOST_VSOCK=y" >> $config_host_mak
 fi
+if test "$vhost_user" = "yes" ; then
+  echo "CONFIG_VHOST_USER=y" >> $config_host_mak
+fi
 if test "$blobs" = "yes" ; then
   echo "INSTALL_BLOBS=yes" >> $config_host_mak
 fi
@@ -6358,7 +6381,9 @@ if supported_kvm_target $target; then
     echo "CONFIG_KVM=y" >> $config_target_mak
     if test "$vhost_net" = "yes" ; then
         echo "CONFIG_VHOST_NET=y" >> $config_target_mak
-        echo "CONFIG_VHOST_NET_TEST_$target_name=y" >> $config_host_mak
+        if test "$vhost_user" = "yes" ; then
+            echo "CONFIG_VHOST_USER_NET_TEST_$target_name=y" >> $config_host_mak
+        fi
     fi
 fi
 if supported_hax_target $target; then
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index acaa70301a..a758630d30 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -43,4 +43,4 @@ CONFIG_VGA=y
 CONFIG_VGA_PCI=y
 CONFIG_IVSHMEM_DEVICE=$(CONFIG_IVSHMEM)
 CONFIG_ROCKER=y
-CONFIG_VHOST_USER_SCSI=$(CONFIG_LINUX)
+CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))
diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
index b227a36179..51191b77df 100644
--- a/default-configs/s390x-softmmu.mak
+++ b/default-configs/s390x-softmmu.mak
@@ -1,6 +1,6 @@
 CONFIG_PCI=y
 CONFIG_VIRTIO_PCI=y
-CONFIG_VHOST_USER_SCSI=$(CONFIG_LINUX)
+CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))
 CONFIG_VIRTIO=y
 CONFIG_SCLPCONSOLE=y
 CONFIG_TERMINAL3270=y
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7af278db55..8f5a3a1cb6 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -255,9 +255,9 @@ check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
 check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
-check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
-ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
-check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
+check-qtest-i386-$(CONFIG_VHOST_USER_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
+ifeq ($(CONFIG_VHOST_USER_NET_TEST_i386),)
+check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
 endif
 check-qtest-i386-y += tests/test-netfilter$(EXESUF)
 check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
-- 
2.14.0.rc0.1.g40ca67566

[Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled

[Marc-André Lureau]

This adds two extra #ifdef that have fairly limited conflict potential.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 4 ++++
 net/Makefile.objs  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e037db63a3..96c4da49e4 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
         }
     }
 
+#ifdef CONFIG_VHOST_USER
     /* Set sane init value. Override when guest acks. */
     if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
         features = vhost_user_get_acked_features(net->nc);
@@ -203,6 +204,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
             goto fail;
         }
     }
+#endif
 
     vhost_net_ack_features(net, features);
 
@@ -414,10 +416,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
     case NET_CLIENT_DRIVER_TAP:
         vhost_net = tap_get_vhost_net(nc);
         break;
+#ifdef CONFIG_VHOST_USER
     case NET_CLIENT_DRIVER_VHOST_USER:
         vhost_net = vhost_user_get_vhost_net(nc);
         assert(vhost_net);
         break;
+#endif
     default:
         break;
     }
diff --git a/net/Makefile.objs b/net/Makefile.objs
index 67ba5e26fb..7cac7ed1e4 100644
--- a/net/Makefile.objs
+++ b/net/Makefile.objs
@@ -3,7 +3,7 @@ common-obj-y += socket.o
 common-obj-y += dump.o
 common-obj-y += eth.o
 common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
-common-obj-$(CONFIG_POSIX) += vhost-user.o
+common-obj-$(CONFIG_VHOST_USER) += vhost-user.o
 common-obj-$(CONFIG_SLIRP) += slirp.o
 common-obj-$(CONFIG_VDE) += vde.o
 common-obj-$(CONFIG_NETMAP) += netmap.o
-- 
2.14.0.rc0.1.g40ca67566

Re: [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user

[Cornelia Huck]

On Fri, 28 Jul 2017 16:13:08 +0200
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> Learn to compile out vhost-user. Keep it enabled by default on
> non-win32, that is assumed to be POSIX. Fail if trying to enable it on
> win32.
> 
> When trying to make a vhost-user netdev, it gives the following error:
> 
> -netdev vhost-user,id=foo,chardev=chr-test: Parameter 'type' expects a netdev backend type
> 
> And similar error with the HMP/QMP monitors.
> 
> While at it, rename CONFIG_VHOST_NET_TEST CONFIG_VHOST_USER_NET_TEST
> since it's a vhost-user specific variable.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/virtio/virtio-pci.c            |  4 ++--
>  configure                         | 29 +++++++++++++++++++++++++++--
>  default-configs/pci.mak           |  2 +-
>  default-configs/s390x-softmmu.mak |  2 +-
>  tests/Makefile.include            |  6 +++---
>  5 files changed, 34 insertions(+), 9 deletions(-)
> 
(...)
> diff --git a/configure b/configure
> index 987f59ba88..efec1a613e 100755
> --- a/configure
> +++ b/configure
> @@ -306,6 +306,7 @@ tcg="yes"
>  vhost_net="no"
>  vhost_scsi="no"
>  vhost_vsock="no"
> +vhost_user=""
>  kvm="no"
>  hax="no"
>  rdma=""
> @@ -1282,6 +1283,15 @@ for opt do
>    ;;
>    --enable-vxhs) vxhs="yes"
>    ;;
> +  --disable-vhost-user) vhost_user="no"
> +  ;;
> +  --enable-vhost-user)
> +      vhost_user="yes"
> +      if test "$mingw32" = "yes" ; then
> +          echo "ERROR: vhost-user isn't available on win32"
> +          exit 1

error_exit?

> +      fi
> +  ;;
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
(...)

> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index b227a36179..51191b77df 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -1,6 +1,6 @@
>  CONFIG_PCI=y
>  CONFIG_VIRTIO_PCI=y
> -CONFIG_VHOST_USER_SCSI=$(CONFIG_LINUX)
> +CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))

Huh. I wonder if anyone actually tried this on s390x?

(The change is fine in the context of this patch, of course.)

>  CONFIG_VIRTIO=y
>  CONFIG_SCLPCONSOLE=y
>  CONFIG_TERMINAL3270=y

Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled

[Michael S. Tsirkin]

On Fri, Jul 28, 2017 at 04:13:09PM +0200, Marc-André Lureau wrote:
> This adds two extra #ifdef that have fairly limited conflict potential.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

OK but why can't we keep all ifdefs in net/ where all the
rest of the features are ifdef'ed?

My concern is no one is going to test this weird configuration
so we might accidentally re-enable it without noticing.
net/net.c is where all command line play happens so
people know a bunch of configs need to be tested when
changing it.


> ---
>  hw/net/vhost_net.c | 4 ++++
>  net/Makefile.objs  | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index e037db63a3..96c4da49e4 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>          }
>      }
>  
> +#ifdef CONFIG_VHOST_USER
>      /* Set sane init value. Override when guest acks. */
>      if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
>          features = vhost_user_get_acked_features(net->nc);
> @@ -203,6 +204,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>              goto fail;
>          }
>      }
> +#endif
>  
>      vhost_net_ack_features(net, features);
>  
> @@ -414,10 +416,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
>      case NET_CLIENT_DRIVER_TAP:
>          vhost_net = tap_get_vhost_net(nc);
>          break;
> +#ifdef CONFIG_VHOST_USER
>      case NET_CLIENT_DRIVER_VHOST_USER:
>          vhost_net = vhost_user_get_vhost_net(nc);
>          assert(vhost_net);
>          break;
> +#endif
>      default:
>          break;
>      }
> diff --git a/net/Makefile.objs b/net/Makefile.objs
> index 67ba5e26fb..7cac7ed1e4 100644
> --- a/net/Makefile.objs
> +++ b/net/Makefile.objs
> @@ -3,7 +3,7 @@ common-obj-y += socket.o
>  common-obj-y += dump.o
>  common-obj-y += eth.o
>  common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
> -common-obj-$(CONFIG_POSIX) += vhost-user.o
> +common-obj-$(CONFIG_VHOST_USER) += vhost-user.o
>  common-obj-$(CONFIG_SLIRP) += slirp.o
>  common-obj-$(CONFIG_VDE) += vde.o
>  common-obj-$(CONFIG_NETMAP) += netmap.o
> -- 
> 2.14.0.rc0.1.g40ca67566

Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled

[Marc-André Lureau]

Hi

----- Original Message -----
> On Fri, Jul 28, 2017 at 04:13:09PM +0200, Marc-André Lureau wrote:
> > This adds two extra #ifdef that have fairly limited conflict potential.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> OK but why can't we keep all ifdefs in net/ where all the
> rest of the features are ifdef'ed?
> 

We would need fallback code for vhost_user_get_* functions (see below).

> My concern is no one is going to test this weird configuration
> so we might accidentally re-enable it without noticing.
> net/net.c is where all command line play happens so
> people know a bunch of configs need to be tested when
> changing it.

I am not sure I understand your argument there. You are afraid we 'accidentaly re-enable' vhost-user... and forget to remove those #ifdef?

I can imagine people don't like having code clutter with #ifdef, but I think when it doesn't change much the logic (as in here with clean per-case switch/if), it is fine, ymmv.

> 
> 
> > ---
> >  hw/net/vhost_net.c | 4 ++++
> >  net/Makefile.objs  | 2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index e037db63a3..96c4da49e4 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > *options)
> >          }
> >      }
> >  
> > +#ifdef CONFIG_VHOST_USER
> >      /* Set sane init value. Override when guest acks. */
> >      if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> >          features = vhost_user_get_acked_features(net->nc);
> > @@ -203,6 +204,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > *options)
> >              goto fail;
> >          }
> >      }
> > +#endif
> >  
> >      vhost_net_ack_features(net, features);
> >  
> > @@ -414,10 +416,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> >      case NET_CLIENT_DRIVER_TAP:
> >          vhost_net = tap_get_vhost_net(nc);
> >          break;
> > +#ifdef CONFIG_VHOST_USER
> >      case NET_CLIENT_DRIVER_VHOST_USER:
> >          vhost_net = vhost_user_get_vhost_net(nc);
> >          assert(vhost_net);
> >          break;
> > +#endif
> >      default:
> >          break;
> >      }
> > diff --git a/net/Makefile.objs b/net/Makefile.objs
> > index 67ba5e26fb..7cac7ed1e4 100644
> > --- a/net/Makefile.objs
> > +++ b/net/Makefile.objs
> > @@ -3,7 +3,7 @@ common-obj-y += socket.o
> >  common-obj-y += dump.o
> >  common-obj-y += eth.o
> >  common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
> > -common-obj-$(CONFIG_POSIX) += vhost-user.o
> > +common-obj-$(CONFIG_VHOST_USER) += vhost-user.o

So we could have something like:

+common-obj-$(not CONFIG_VHOST_USER) += vhost-user-stubs.o

?

> >  common-obj-$(CONFIG_SLIRP) += slirp.o
> >  common-obj-$(CONFIG_VDE) += vde.o
> >  common-obj-$(CONFIG_NETMAP) += netmap.o
> > --
> > 2.14.0.rc0.1.g40ca67566
> 

Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled

[Michael S. Tsirkin]

On Tue, Aug 01, 2017 at 12:15:16PM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Fri, Jul 28, 2017 at 04:13:09PM +0200, Marc-André Lureau wrote:
> > > This adds two extra #ifdef that have fairly limited conflict potential.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > OK but why can't we keep all ifdefs in net/ where all the
> > rest of the features are ifdef'ed?
> > 
> 
> We would need fallback code for vhost_user_get_* functions (see below).

Not if you merely add ifdefs around name arrays in net/net.c
I think.

> > My concern is no one is going to test this weird configuration
> > so we might accidentally re-enable it without noticing.
> > net/net.c is where all command line play happens so
> > people know a bunch of configs need to be tested when
> > changing it.
> 
> I am not sure I understand your argument there. You are afraid we 'accidentaly re-enable' vhost-user... and forget to remove those #ifdef?
> 
> I can imagine people don't like having code clutter with #ifdef, but I think when it doesn't change much the logic (as in here with clean per-case switch/if), it is fine, ymmv.

Let's assume all this code is refactored, we'd have to test
with ifdefs.


> > 
> > 
> > > ---
> > >  hw/net/vhost_net.c | 4 ++++
> > >  net/Makefile.objs  | 2 +-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index e037db63a3..96c4da49e4 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > > *options)
> > >          }
> > >      }
> > >  
> > > +#ifdef CONFIG_VHOST_USER
> > >      /* Set sane init value. Override when guest acks. */
> > >      if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > >          features = vhost_user_get_acked_features(net->nc);
> > > @@ -203,6 +204,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > > *options)
> > >              goto fail;
> > >          }
> > >      }
> > > +#endif
> > >  
> > >      vhost_net_ack_features(net, features);
> > >  
> > > @@ -414,10 +416,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > >      case NET_CLIENT_DRIVER_TAP:
> > >          vhost_net = tap_get_vhost_net(nc);
> > >          break;
> > > +#ifdef CONFIG_VHOST_USER
> > >      case NET_CLIENT_DRIVER_VHOST_USER:
> > >          vhost_net = vhost_user_get_vhost_net(nc);
> > >          assert(vhost_net);
> > >          break;
> > > +#endif
> > >      default:
> > >          break;
> > >      }
> > > diff --git a/net/Makefile.objs b/net/Makefile.objs
> > > index 67ba5e26fb..7cac7ed1e4 100644
> > > --- a/net/Makefile.objs
> > > +++ b/net/Makefile.objs
> > > @@ -3,7 +3,7 @@ common-obj-y += socket.o
> > >  common-obj-y += dump.o
> > >  common-obj-y += eth.o
> > >  common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
> > > -common-obj-$(CONFIG_POSIX) += vhost-user.o
> > > +common-obj-$(CONFIG_VHOST_USER) += vhost-user.o
> 
> So we could have something like:
> 
> +common-obj-$(not CONFIG_VHOST_USER) += vhost-user-stubs.o
> 
> ?
> 
> > >  common-obj-$(CONFIG_SLIRP) += slirp.o
> > >  common-obj-$(CONFIG_VDE) += vde.o
> > >  common-obj-$(CONFIG_NETMAP) += netmap.o
> > > --
> > > 2.14.0.rc0.1.g40ca67566
> > 

Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled

[Marc-André Lureau]

----- Original Message -----
> On Tue, Aug 01, 2017 at 12:15:16PM -0400, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Original Message -----
> > > On Fri, Jul 28, 2017 at 04:13:09PM +0200, Marc-André Lureau wrote:
> > > > This adds two extra #ifdef that have fairly limited conflict potential.
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > OK but why can't we keep all ifdefs in net/ where all the
> > > rest of the features are ifdef'ed?
> > > 
> > 
> > We would need fallback code for vhost_user_get_* functions (see below).
> 
> Not if you merely add ifdefs around name arrays in net/net.c
> I think.

vhost_user_get_* are defined in net/vhost-user.c, linked from hw/net/vhost_net.c

> 
> > > My concern is no one is going to test this weird configuration
> > > so we might accidentally re-enable it without noticing.
> > > net/net.c is where all command line play happens so
> > > people know a bunch of configs need to be tested when
> > > changing it.
> > 
> > I am not sure I understand your argument there. You are afraid we
> > 'accidentaly re-enable' vhost-user... and forget to remove those #ifdef?
> > 
> > I can imagine people don't like having code clutter with #ifdef, but I
> > think when it doesn't change much the logic (as in here with clean
> > per-case switch/if), it is fine, ymmv.
> 
> Let's assume all this code is refactored, we'd have to test
> with ifdefs.

This is true whether there is one or ten #ifdef, isn't it?

It only gets more complicated the more #ifdef there is. But here it's fairly limited impact since it's just 2 more #ifdef for the proposed CONFIG_VHOST_USER from previous patch.

> 
> > > 
> > > 
> > > > ---
> > > >  hw/net/vhost_net.c | 4 ++++
> > > >  net/Makefile.objs  | 2 +-
> > > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index e037db63a3..96c4da49e4 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > > > *options)
> > > >          }
> > > >      }
> > > >  
> > > > +#ifdef CONFIG_VHOST_USER
> > > >      /* Set sane init value. Override when guest acks. */
> > > >      if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > > >          features = vhost_user_get_acked_features(net->nc);
> > > > @@ -203,6 +204,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > > > *options)
> > > >              goto fail;
> > > >          }
> > > >      }
> > > > +#endif
> > > >  
> > > >      vhost_net_ack_features(net, features);
> > > >  
> > > > @@ -414,10 +416,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > > >      case NET_CLIENT_DRIVER_TAP:
> > > >          vhost_net = tap_get_vhost_net(nc);
> > > >          break;
> > > > +#ifdef CONFIG_VHOST_USER
> > > >      case NET_CLIENT_DRIVER_VHOST_USER:
> > > >          vhost_net = vhost_user_get_vhost_net(nc);
> > > >          assert(vhost_net);
> > > >          break;
> > > > +#endif
> > > >      default:
> > > >          break;
> > > >      }
> > > > diff --git a/net/Makefile.objs b/net/Makefile.objs
> > > > index 67ba5e26fb..7cac7ed1e4 100644
> > > > --- a/net/Makefile.objs
> > > > +++ b/net/Makefile.objs
> > > > @@ -3,7 +3,7 @@ common-obj-y += socket.o
> > > >  common-obj-y += dump.o
> > > >  common-obj-y += eth.o
> > > >  common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
> > > > -common-obj-$(CONFIG_POSIX) += vhost-user.o
> > > > +common-obj-$(CONFIG_VHOST_USER) += vhost-user.o
> > 
> > So we could have something like:
> > 
> > +common-obj-$(not CONFIG_VHOST_USER) += vhost-user-stubs.o
> > 
> > ?
> > 
> > > >  common-obj-$(CONFIG_SLIRP) += slirp.o
> > > >  common-obj-$(CONFIG_VDE) += vde.o
> > > >  common-obj-$(CONFIG_NETMAP) += netmap.o
> > > > --
> > > > 2.14.0.rc0.1.g40ca67566
> > > 
> 

Re: [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user

[Michael S. Tsirkin]

On Fri, Jul 28, 2017 at 04:13:08PM +0200, Marc-André Lureau wrote:
> Learn to compile out vhost-user. Keep it enabled by default on
> non-win32, that is assumed to be POSIX. Fail if trying to enable it on
> win32.
> 
> When trying to make a vhost-user netdev, it gives the following error:
> 
> -netdev vhost-user,id=foo,chardev=chr-test: Parameter 'type' expects a netdev backend type
> 
> And similar error with the HMP/QMP monitors.
> 
> While at it, rename CONFIG_VHOST_NET_TEST CONFIG_VHOST_USER_NET_TEST
> since it's a vhost-user specific variable.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

OK so pls address Cornelia's comment and post just patch 1 as
it seems appropriate for 2.10.

> ---
>  hw/virtio/virtio-pci.c            |  4 ++--
>  configure                         | 29 +++++++++++++++++++++++++++--
>  default-configs/pci.mak           |  2 +-
>  default-configs/s390x-softmmu.mak |  2 +-
>  tests/Makefile.include            |  6 +++---
>  5 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 5d14bd66dc..8b0d6b69cd 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2135,7 +2135,7 @@ static const TypeInfo vhost_scsi_pci_info = {
>  };
>  #endif
>  
> -#ifdef CONFIG_LINUX
> +#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
>  /* vhost-user-scsi-pci */
>  static Property vhost_user_scsi_pci_properties[] = {
>      DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
> @@ -2665,7 +2665,7 @@ static void virtio_pci_register_types(void)
>  #ifdef CONFIG_VHOST_SCSI
>      type_register_static(&vhost_scsi_pci_info);
>  #endif
> -#ifdef CONFIG_LINUX
> +#if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
>      type_register_static(&vhost_user_scsi_pci_info);
>  #endif
>  #ifdef CONFIG_VHOST_VSOCK
> diff --git a/configure b/configure
> index 987f59ba88..efec1a613e 100755
> --- a/configure
> +++ b/configure
> @@ -306,6 +306,7 @@ tcg="yes"
>  vhost_net="no"
>  vhost_scsi="no"
>  vhost_vsock="no"
> +vhost_user=""
>  kvm="no"
>  hax="no"
>  rdma=""
> @@ -1282,6 +1283,15 @@ for opt do
>    ;;
>    --enable-vxhs) vxhs="yes"
>    ;;
> +  --disable-vhost-user) vhost_user="no"
> +  ;;
> +  --enable-vhost-user)
> +      vhost_user="yes"
> +      if test "$mingw32" = "yes" ; then
> +          echo "ERROR: vhost-user isn't available on win32"
> +          exit 1
> +      fi
> +  ;;
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
> @@ -1290,6 +1300,14 @@ for opt do
>    esac
>  done
>  
> +if test "$vhost_user" = ""; then
> +    if test "$mingw32" = "yes" ; then
> +        vhost_user="no"
> +    else
> +        vhost_user="yes"
> +    fi
> +fi
> +
>  case "$cpu" in
>      ppc)
>             CPU_CFLAGS="-m32"
> @@ -1518,6 +1536,7 @@ disabled with --disable-FEATURE, default is enabled if available:
>    tools           build qemu-io, qemu-nbd and qemu-image tools
>    vxhs            Veritas HyperScale vDisk backend support
>    crypto-afalg    Linux AF_ALG crypto backend driver
> +  vhost-user      vhost-user support
>  
>  NOTE: The object files are built at the place where configure is launched
>  EOF
> @@ -5348,6 +5367,7 @@ echo "libcap-ng support $cap_ng"
>  echo "vhost-net support $vhost_net"
>  echo "vhost-scsi support $vhost_scsi"
>  echo "vhost-vsock support $vhost_vsock"
> +echo "vhost-user support $vhost_user"
>  echo "Trace backends    $trace_backends"
>  if have_backend "simple"; then
>  echo "Trace output file $trace_file-<pid>"
> @@ -5757,12 +5777,15 @@ fi
>  if test "$vhost_scsi" = "yes" ; then
>    echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
>  fi
> -if test "$vhost_net" = "yes" ; then
> +if test "$vhost_net" = "yes" -a "$vhost_user" = "yes"; then
>    echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
>  fi
>  if test "$vhost_vsock" = "yes" ; then
>    echo "CONFIG_VHOST_VSOCK=y" >> $config_host_mak
>  fi
> +if test "$vhost_user" = "yes" ; then
> +  echo "CONFIG_VHOST_USER=y" >> $config_host_mak
> +fi
>  if test "$blobs" = "yes" ; then
>    echo "INSTALL_BLOBS=yes" >> $config_host_mak
>  fi
> @@ -6358,7 +6381,9 @@ if supported_kvm_target $target; then
>      echo "CONFIG_KVM=y" >> $config_target_mak
>      if test "$vhost_net" = "yes" ; then
>          echo "CONFIG_VHOST_NET=y" >> $config_target_mak
> -        echo "CONFIG_VHOST_NET_TEST_$target_name=y" >> $config_host_mak
> +        if test "$vhost_user" = "yes" ; then
> +            echo "CONFIG_VHOST_USER_NET_TEST_$target_name=y" >> $config_host_mak
> +        fi
>      fi
>  fi
>  if supported_hax_target $target; then
> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> index acaa70301a..a758630d30 100644
> --- a/default-configs/pci.mak
> +++ b/default-configs/pci.mak
> @@ -43,4 +43,4 @@ CONFIG_VGA=y
>  CONFIG_VGA_PCI=y
>  CONFIG_IVSHMEM_DEVICE=$(CONFIG_IVSHMEM)
>  CONFIG_ROCKER=y
> -CONFIG_VHOST_USER_SCSI=$(CONFIG_LINUX)
> +CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))
> diff --git a/default-configs/s390x-softmmu.mak b/default-configs/s390x-softmmu.mak
> index b227a36179..51191b77df 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -1,6 +1,6 @@
>  CONFIG_PCI=y
>  CONFIG_VIRTIO_PCI=y
> -CONFIG_VHOST_USER_SCSI=$(CONFIG_LINUX)
> +CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))
>  CONFIG_VIRTIO=y
>  CONFIG_SCLPCONSOLE=y
>  CONFIG_TERMINAL3270=y
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 7af278db55..8f5a3a1cb6 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -255,9 +255,9 @@ check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
> -check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
> -ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> -check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
> +check-qtest-i386-$(CONFIG_VHOST_USER_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
> +ifeq ($(CONFIG_VHOST_USER_NET_TEST_i386),)
> +check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
>  endif
>  check-qtest-i386-y += tests/test-netfilter$(EXESUF)
>  check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
> -- 
> 2.14.0.rc0.1.g40ca67566