| Message ID | 52081254.597343.1376909324949.JavaMail.root@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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; > >
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
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
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
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
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;
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(-)