Re: [PATCH for-5.0 3/4] Remove the core bluetooth code

From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
	"Thomas Huth" <thuth@redhat.com>,
	libvir-list@redhat.com,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH for-5.0 3/4] Remove the core bluetooth code
Date: Wed, 5 Feb 2020 18:40:31 +0100	[thread overview]
Message-ID: <CAL1e-=gd-ymYd3_cods8BAtwbQeJFodmnmsNo-gLx381rXHKxQ@mail.gmail.com> (raw)
In-Reply-To: <CAAdtpL7vPCQ8k91nvBWc_NLrmhu6OPVprUSL8uzi_v6HMiSrnw@mail.gmail.com>

On Sat, Feb 1, 2020 at 7:53 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On Sat, Feb 1, 2020 at 7:51 PM Thomas Huth <thuth@redhat.com> wrote:
> > On 01/02/2020 17.09, Philippe Mathieu-Daudé wrote:
> > > On 11/20/19 10:10 AM, Thomas Huth wrote:
> > >> It's been deprecated since QEMU v3.1. We've explicitly asked in the
> > >> deprecation message that people should speak up on qemu-devel in case
> > >> they are still actively using the bluetooth part of QEMU, but nobody
> > >> ever replied that they are really still using it.
> > >>
> > >> I've tried it on my own to use this bluetooth subsystem for one of my
> > >> guests, but I was also not able to get it running anymore: When I was
> > >> trying to pass-through a real bluetooth device, either the guest did
> > >> not see the device at all, or the guest crashed.
> > >>
> > >> Even worse for the emulated device: When running
> > >>
> > >>  qemu-system-x86_64 -bt device:keyboard
> > >>
> > >> QEMU crashes once you hit a key.
> > >>
> > >> So it seems like the bluetooth stack is not only neglected, it is
> > >> completely bitrotten, as far as I can tell. The only attention that
> > >> this code got during the past years were some CVEs that have been
> > >> spotted there. So this code is a burden for the developers, without
> > >> any real benefit anymore. Time to remove it.
> > >>
> > >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > >> ---
> > >>  Makefile.objs        |    2 -
> > >>  bt-host.c            |  198 ----
> > >>  bt-vhci.c            |  167 ----
> > >>  configure            |   31 -
> > >>  hw/Kconfig           |    1 -
> > >>  hw/Makefile.objs     |    1 -
> > >>  hw/bt/Kconfig        |    2 -
> > >>  hw/bt/Makefile.objs  |    3 -
> > >>  hw/bt/core.c         |  143 ---
> > >>  hw/bt/hci-csr.c      |  512 ----------
> > >>  hw/bt/hci.c          | 2263 ------------------------------------------
> > >>  hw/bt/hid.c          |  553 -----------
> > >>  hw/bt/l2cap.c        | 1367 -------------------------
> > >>  hw/bt/sdp.c          |  989 ------------------
> > >>  include/hw/bt.h      | 2177 ----------------------------------------
> > >>  include/sysemu/bt.h  |   20 -
> > >>  qemu-deprecated.texi |    7 -
> > >>  qemu-options.hx      |   79 --
> > >>  vl.c                 |  136 ---
> > >>  19 files changed, 8651 deletions(-)
> > >>  delete mode 100644 bt-host.c
> > >>  delete mode 100644 bt-vhci.c
> > >>  delete mode 100644 hw/bt/Kconfig
> > >>  delete mode 100644 hw/bt/Makefile.objs
> > >>  delete mode 100644 hw/bt/core.c
> > >>  delete mode 100644 hw/bt/hci-csr.c
> > >>  delete mode 100644 hw/bt/hci.c
> > >>  delete mode 100644 hw/bt/hid.c
> > >>  delete mode 100644 hw/bt/l2cap.c
> > >>  delete mode 100644 hw/bt/sdp.c
> > >>  delete mode 100644 include/hw/bt.h
> > >>  delete mode 100644 include/sysemu/bt.h
> > >>
> > > [...]> diff --git a/configure b/configure
> > >> index 6099be1d84..ecce4ada2d 100755
> > >> --- a/configure
> > >> +++ b/configure
> > >> @@ -349,7 +349,6 @@ unset target_list_exclude
> > >>  # Distributions want to ensure that several features are compiled in, and it
> > >>  # is impossible without a --enable-foo that exits if a feature is not found.
> > >>
> > >> -bluez=""
> > >>  brlapi=""
> > >>  curl=""
> > >>  curses=""
> > >> @@ -1151,10 +1150,6 @@ for opt do
> > >>    ;;
> > >>    --enable-brlapi) brlapi="yes"
> > >>    ;;
> > >> -  --disable-bluez) bluez="no"
> > >> -  ;;
> > >> -  --enable-bluez) bluez="yes"
> > >> -  ;;
> > >
> > > Now than I'm bisecting over this commit, I realize removing this
> > > option was not a good idea, we should have done like commit
> > > cb6414dfec8 or 315d3184525:
> > >
> > >   @@ -886,10 +885,6 @@ for opt do
> > >   -  --disable-uuid) uuid="no"
> > >   -  ;;
> > >   -  --enable-uuid) uuid="yes"
> > >   -  ;;
> > >   ...
> > >   +  --enable-uuid|--disable-uuid)
> > >   +      echo "$0: $opt is obsolete, UUID support is always built" >&2
> > >   +  ;;
> >
> > Looks trivial ... so if it bugs you, just send a patch?
>
> I thought about it but this won't fix much, it is too late now.
>
> I simply wanted to share this bugged me so we try to avoid doing the
> same mistake again.
>

I vote for addition of a change similar to what Philippe described.

Furthermore, it looks to me the correct way would be to now do full
deprecation of --enable-bluez and --disable-bluez. This means adding
this to documentation (not related to bluetooth devices support), not
only a change in "configure". This also means that only after two next
full cycles these options could be removed.

True, this could have been done together with bluetooth devices
support deprecation (and in that case we could have deleted these
options right away), but it wasn't. Users don't have a crystal ball to
know that we assumed that --enable-bluez and --disable-bluez were part
of bluetooth devices support, and could rightfully complain about an
abrupt elimination of a compile time option.

This is my understanding, at least.

What do you think?

Aleksandar