* [Qemu-devel] [PATCH v2 0/2] Add --disable-vhost-user @ 2017-07-28 14:13 Marc-André Lureau 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user Marc-André Lureau 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled Marc-André Lureau 0 siblings, 2 replies; 9+ messages in thread From: Marc-André Lureau @ 2017-07-28 14:13 UTC (permalink / raw) To: qemu-devel; +Cc: philippe.mathieu.daude, mst, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user 2017-07-28 14:13 [Qemu-devel] [PATCH v2 0/2] Add --disable-vhost-user Marc-André Lureau @ 2017-07-28 14:13 ` Marc-André Lureau 2017-08-01 8:15 ` Cornelia Huck 2017-08-03 1:18 ` Michael S. Tsirkin 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled Marc-André Lureau 1 sibling, 2 replies; 9+ messages in thread From: Marc-André Lureau @ 2017-07-28 14:13 UTC (permalink / raw) To: qemu-devel Cc: philippe.mathieu.daude, mst, Marc-André Lureau, Cornelia Huck, Christian Borntraeger, Alexander Graf 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user Marc-André Lureau @ 2017-08-01 8:15 ` Cornelia Huck 2017-08-03 1:18 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Cornelia Huck @ 2017-08-01 8:15 UTC (permalink / raw) To: Marc-André Lureau Cc: qemu-devel, philippe.mathieu.daude, mst, Christian Borntraeger, Alexander Graf 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user Marc-André Lureau 2017-08-01 8:15 ` Cornelia Huck @ 2017-08-03 1:18 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2017-08-03 1:18 UTC (permalink / raw) To: Marc-André Lureau Cc: qemu-devel, philippe.mathieu.daude, Cornelia Huck, Christian Borntraeger, Alexander Graf 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled 2017-07-28 14:13 [Qemu-devel] [PATCH v2 0/2] Add --disable-vhost-user Marc-André Lureau 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user Marc-André Lureau @ 2017-07-28 14:13 ` Marc-André Lureau 2017-08-01 16:03 ` Michael S. Tsirkin 1 sibling, 1 reply; 9+ messages in thread From: Marc-André Lureau @ 2017-07-28 14:13 UTC (permalink / raw) To: qemu-devel Cc: philippe.mathieu.daude, mst, Marc-André Lureau, Jason Wang 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled Marc-André Lureau @ 2017-08-01 16:03 ` Michael S. Tsirkin 2017-08-01 16:15 ` Marc-André Lureau 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2017-08-01 16:03 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, philippe.mathieu.daude, Jason Wang 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled 2017-08-01 16:03 ` Michael S. Tsirkin @ 2017-08-01 16:15 ` Marc-André Lureau 2017-08-01 16:25 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Marc-André Lureau @ 2017-08-01 16:15 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel, philippe mathieu daude, Jason Wang 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled 2017-08-01 16:15 ` Marc-André Lureau @ 2017-08-01 16:25 ` Michael S. Tsirkin 2017-08-01 17:07 ` Marc-André Lureau 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2017-08-01 16:25 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, philippe mathieu daude, Jason Wang 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled 2017-08-01 16:25 ` Michael S. Tsirkin @ 2017-08-01 17:07 ` Marc-André Lureau 0 siblings, 0 replies; 9+ messages in thread From: Marc-André Lureau @ 2017-08-01 17:07 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel, philippe mathieu daude, Jason Wang ----- 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 > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-03 1:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-28 14:13 [Qemu-devel] [PATCH v2 0/2] Add --disable-vhost-user Marc-André Lureau 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user Marc-André Lureau 2017-08-01 8:15 ` Cornelia Huck 2017-08-03 1:18 ` Michael S. Tsirkin 2017-07-28 14:13 ` [Qemu-devel] [PATCH v2 2/2] build-sys: do not compile net/vhost-user.c if vhost-user is disabled Marc-André Lureau 2017-08-01 16:03 ` Michael S. Tsirkin 2017-08-01 16:15 ` Marc-André Lureau 2017-08-01 16:25 ` Michael S. Tsirkin 2017-08-01 17:07 ` Marc-André Lureau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).