Re: [Qemu-ppc] commit 094b287 "Add USB option in machine options" broke i386 -usb

[Alexander Graf]

On 22.11.2012, at 17:39, Peter Maydell wrote:

> On 22 November 2012 16:34, Li Zhang <address@hidden> wrote:
>> On 2012年11月23日 00:18, Peter Maydell wrote:
>>> 
>>> On 29 October 2012 10:25, Alexander Graf <address@hidden> wrote:
>>>> 
>>>> From: address@hidden <address@hidden>
>>>> 
>>>> When -usb option is used, global varible usb_enabled is set.
>>>> And all the plaform will create one USB controller according
>>>> to this variable. In fact, global varibles make code hard
>>>> to read.
>>>> 
>>>> So this patch is to remove global variable usb_enabled and
>>>> add USB option in machine options. All the plaforms will get
>>>> USB option value from machine options.
>>>> 
>>>> USB option of machine options will be set either by:
>>>>   * -usb
>>>>   * -machine type=pseries,usb=on
>>>> 
>>>> Both these ways can work now. They both set USB option in
>>>> machine options. In the future, the first way will be removed.
>>> 
>>> This commit breaks "qemu-system-i386 -usb", which used to
>>> create a USB bus and now does not. Example to reproduce:
>>> 
>>> $ dd if=/dev/zero bs=4096 count=1024 of=usb.img
>>> $ ./i386-softmmu/qemu-system-i386 -clock unix -monitor stdio -usb
>>> -drive if=none,file=usb.img,id=myusb  -device
>>> usb-storage,id=myusb,drive=myusb,removable=on
>> 
>> Hi Peter,
>> 
>> I am sorry to cause this problem.
>> 
>> I couldn't see  the machine's option in your command line.
>> such as, -machine type=pc
>> 
>> As it describes, you need to pass this option to enable USB.
>> Because USB option is set by machine's option.
> 
> We use "-usb" to enable USB in this command line. That has
> to continue to work, for back compatibility. I think your
> code to try to support -usb is broken:
>  case QEMU_OPTION_usb:
>    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> 
> will return NULL if there are no user-set machine opts
> at this point, so we never go on to try to set the usb
> machine opt to true.

That logic should probably be changed. All the checks whether machine_opts 
exist are really quite annoying.


Alex