[Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Miroslav Rezanina]

usb-bt-dongle device can't be disabled as there's dependency in vl.c file. This patch add preprocesor condition to be able to disable it.

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
---
 hw/usb/Makefile.objs |  1 -
 vl.c                 | 18 ++++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index f9695e7..8892ffd 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -20,7 +20,6 @@ common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
 common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
 
 # FIXME: make configurable too
-CONFIG_USB_BLUETOOTH := y
 common-obj-$(CONFIG_USB_BLUETOOTH)    += dev-bluetooth.o
 
 ifeq ($(CONFIG_USB_SMARTCARD),y)
diff --git a/vl.c b/vl.c
index f422a1c..4330b6d 100644
--- a/vl.c
+++ b/vl.c
@@ -1526,8 +1526,10 @@ static void configure_msg(QemuOpts *opts)
 
 static int usb_device_add(const char *devname)
 {
-    const char *p;
     USBDevice *dev = NULL;
+#if  defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX)
+    const char *p;
+#endif
 
     if (!usb_enabled(false)) {
         return -1;
@@ -1543,15 +1545,23 @@ static int usb_device_add(const char *devname)
     /* only the linux version is qdev-ified, usb-bsd still needs this */
     if (strstart(devname, "host:", &p)) {
         dev = usb_host_device_open(usb_bus_find(-1), p);
-    } else
+        goto devtest;
+    } 
 #endif
+#ifdef CONFIG_USB_BLUETOOTH
     if (!strcmp(devname, "bt") || strstart(devname, "bt:", &p)) {
         dev = usb_bt_init(usb_bus_find(-1),
                           devname[2] ? hci_init(p)
                                      : bt_new_hci(qemu_find_bt_vlan(0)));
-    } else {
-        return -1;
+        goto devtest;
     }
+#endif
+
+    return -1;
+
+#if  defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX)
+devtest:
+#endif
     if (!dev)
         return -1;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Andreas Färber]

Am 19.08.2013 12:48, schrieb Miroslav Rezanina:
> usb-bt-dongle device can't be disabled as there's dependency in vl.c file. This patch add preprocesor condition to be able to disable it.

Please limit to 76 chars per line (check `git log` output).

> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
>  hw/usb/Makefile.objs |  1 -
>  vl.c                 | 18 ++++++++++++++----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index f9695e7..8892ffd 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -20,7 +20,6 @@ common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
>  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>  
>  # FIXME: make configurable too
> -CONFIG_USB_BLUETOOTH := y

You probably should delete the FIXME alongside?

>  common-obj-$(CONFIG_USB_BLUETOOTH)    += dev-bluetooth.o
>  
>  ifeq ($(CONFIG_USB_SMARTCARD),y)
> diff --git a/vl.c b/vl.c
> index f422a1c..4330b6d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1526,8 +1526,10 @@ static void configure_msg(QemuOpts *opts)
>  
>  static int usb_device_add(const char *devname)
>  {
> -    const char *p;
>      USBDevice *dev = NULL;
> +#if  defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX)

Double space.

> +    const char *p;
> +#endif
>  
>      if (!usb_enabled(false)) {
>          return -1;
> @@ -1543,15 +1545,23 @@ static int usb_device_add(const char *devname)
>      /* only the linux version is qdev-ified, usb-bsd still needs this */
>      if (strstart(devname, "host:", &p)) {
>          dev = usb_host_device_open(usb_bus_find(-1), p);
> -    } else
> +        goto devtest;
> +    } 
>  #endif
> +#ifdef CONFIG_USB_BLUETOOTH
>      if (!strcmp(devname, "bt") || strstart(devname, "bt:", &p)) {
>          dev = usb_bt_init(usb_bus_find(-1),
>                            devname[2] ? hci_init(p)
>                                       : bt_new_hci(qemu_find_bt_vlan(0)));
> -    } else {
> -        return -1;
> +        goto devtest;
>      }
> +#endif
> +
> +    return -1;
> +
> +#if  defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX)
> +devtest:
> +#endif

Why put only the label in an #if block? The below is dead code if not
reached via devtest.

>      if (!dev)
>          return -1;
>  

Let's also not forget to CC Gerd as USB maintainer.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Paolo Bonzini]

Il 19/08/2013 12:48, Miroslav Rezanina ha scritto:
> usb-bt-dongle device can't be disabled as there's dependency in vl.c file. This patch add preprocesor condition to be able to disable it.
> 
> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
> ---
>  hw/usb/Makefile.objs |  1 -
>  vl.c                 | 18 ++++++++++++++----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index f9695e7..8892ffd 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -20,7 +20,6 @@ common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
>  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>  
>  # FIXME: make configurable too
> -CONFIG_USB_BLUETOOTH := y
>  common-obj-$(CONFIG_USB_BLUETOOTH)    += dev-bluetooth.o
>  
>  ifeq ($(CONFIG_USB_SMARTCARD),y)
> diff --git a/vl.c b/vl.c
> index f422a1c..4330b6d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1526,8 +1526,10 @@ static void configure_msg(QemuOpts *opts)
>  
>  static int usb_device_add(const char *devname)
>  {
> -    const char *p;
>      USBDevice *dev = NULL;
> +#if  defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX)
> +    const char *p;
> +#endif
>  
>      if (!usb_enabled(false)) {
>          return -1;
> @@ -1543,15 +1545,23 @@ static int usb_device_add(const char *devname)
>      /* only the linux version is qdev-ified, usb-bsd still needs this */
>      if (strstart(devname, "host:", &p)) {
>          dev = usb_host_device_open(usb_bus_find(-1), p);
> -    } else
> +        goto devtest;
> +    } 

No need for the goto, since the following "if"s will fail...

>  #endif
> +#ifdef CONFIG_USB_BLUETOOTH
>      if (!strcmp(devname, "bt") || strstart(devname, "bt:", &p)) {
>          dev = usb_bt_init(usb_bus_find(-1),
>                            devname[2] ? hci_init(p)
>                                       : bt_new_hci(qemu_find_bt_vlan(0)));
> -    } else {
> -        return -1;
> +        goto devtest;

... same here...

>      }
> +#endif
> +
> +    return -1;

... and no need for this return either: if you get here, dev will be
NULL and the "if" just below will fail.

Paolo

> +#if  defined(CONFIG_USB_BLUETOOTH) || !defined(CONFIG_LINUX)
> +devtest:
> +#endif
>      if (!dev)
>          return -1;
>  
> 

Re: [Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Laszlo Ersek]

On 08/19/13 14:31, Andreas Färber wrote:
> Am 19.08.2013 12:48, schrieb Miroslav Rezanina:
>> usb-bt-dongle device can't be disabled as there's dependency in vl.c file. This patch add preprocesor condition to be able to disable it.
> 
> Please limit to 76 chars per line (check `git log` output).
> 
>>
>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>> ---
>>  hw/usb/Makefile.objs |  1 -
>>  vl.c                 | 18 ++++++++++++++----
>>  2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
>> index f9695e7..8892ffd 100644
>> --- a/hw/usb/Makefile.objs
>> +++ b/hw/usb/Makefile.objs
>> @@ -20,7 +20,6 @@ common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
>>  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>>  
>>  # FIXME: make configurable too
>> -CONFIG_USB_BLUETOOTH := y
> 
> You probably should delete the FIXME alongside?

What's everyone's opinion about CONFIG_USB_BLUETOOTH=y disappearing from
the default build?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Andreas Färber]

Am 19.08.2013 15:30, schrieb Laszlo Ersek:
> On 08/19/13 14:31, Andreas Färber wrote:
>> Am 19.08.2013 12:48, schrieb Miroslav Rezanina:
>>> usb-bt-dongle device can't be disabled as there's dependency in vl.c file. This patch add preprocesor condition to be able to disable it.
>>
>> Please limit to 76 chars per line (check `git log` output).
>>
>>>
>>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>>> ---
>>>  hw/usb/Makefile.objs |  1 -
>>>  vl.c                 | 18 ++++++++++++++----
>>>  2 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
>>> index f9695e7..8892ffd 100644
>>> --- a/hw/usb/Makefile.objs
>>> +++ b/hw/usb/Makefile.objs
>>> @@ -20,7 +20,6 @@ common-obj-$(CONFIG_USB_SERIAL)       += dev-serial.o
>>>  common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>>>  
>>>  # FIXME: make configurable too
>>> -CONFIG_USB_BLUETOOTH := y
>>
>> You probably should delete the FIXME alongside?
> 
> What's everyone's opinion about CONFIG_USB_BLUETOOTH=y disappearing from
> the default build?

By my reading of `git grep CONFIG_USB_BLUETOOTH` it isn't disappearing,
check default-configs/usb.mak. All targets that include usb.mak will
have CONFIG_USB_BLUETOOTH.

It's only used in the build system and with this patch in vl.c, so
assuming that Miroslav has checked that the build succeeds for all
targets, this should be fine, I guess.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Paolo Bonzini]

Il 19/08/2013 15:41, Andreas Färber ha scritto:
> By my reading of `git grep CONFIG_USB_BLUETOOTH` it isn't disappearing,
> check default-configs/usb.mak. All targets that include usb.mak will
> have CONFIG_USB_BLUETOOTH.
> 
> It's only used in the build system and with this patch in vl.c, so
> assuming that Miroslav has checked that the build succeeds for all
> targets, this should be fine, I guess.

Actually, CONFIG_* symbols for devices are _not_ exposed to vl.c.  Blue
Swirl specifically wanted that to happen.  So I'm afraid Mirek's patch
breaks "-usb bt" even if CONFIG_USB_BLUETOOTH is set to y.

Paolo

Re: [Qemu-devel] [PATCH] Make usb-bt-dongle configurable

[Gerd Hoffmann]

On Mo, 2013-08-19 at 15:58 +0200, Paolo Bonzini wrote:
> Il 19/08/2013 15:41, Andreas Färber ha scritto:
> > By my reading of `git grep CONFIG_USB_BLUETOOTH` it isn't disappearing,
> > check default-configs/usb.mak. All targets that include usb.mak will
> > have CONFIG_USB_BLUETOOTH.
> > 
> > It's only used in the build system and with this patch in vl.c, so
> > assuming that Miroslav has checked that the build succeeds for all
> > targets, this should be fine, I guess.
> 
> Actually, CONFIG_* symbols for devices are _not_ exposed to vl.c.  Blue
> Swirl specifically wanted that to happen.  So I'm afraid Mirek's patch
> breaks "-usb bt" even if CONFIG_USB_BLUETOOTH is set to y.

"-usbdevice bt", to be exact.

I'd suggest to switch bluetooth over to use usb_legacy_register() to
parse+handle the legacy -usbdevice cmd line option and zap the vl.c
dependency this way.

cheers,
  Gerd