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

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

[Peter Maydell]

On 29 October 2012 10:25, Alexander Graf <agraf@suse.de> wrote:
> From: zhlcindy@gmail.com <zhlcindy@gmail.com>
>
> 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

should boot up, but now fails with:
qemu-system-i386: -device
usb-storage,id=myusb,drive=myusb,removable=on: No 'usb-bus' bus found
for device 'usb-storage'

This worked in 1.2 so it is a regression. In particular it
presumably means that trying to migrate a VM from 1.2 qemu
that used USB will fail.

-- PMM

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

[Li Zhang]

On 2012年11月23日 00:18, Peter Maydell wrote:
> On 29 October 2012 10:25, Alexander Graf <agraf@suse.de> wrote:
>> From: zhlcindy@gmail.com <zhlcindy@gmail.com>
>>
>> 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.

Thanks. :-)
>
> should boot up, but now fails with:
> qemu-system-i386: -device
> usb-storage,id=myusb,drive=myusb,removable=on: No 'usb-bus' bus found
> for device 'usb-storage'
>
> This worked in 1.2 so it is a regression. In particular it
> presumably means that trying to migrate a VM from 1.2 qemu
> that used USB will fail.
>
> -- PMM
>


-- 

Li Zhang
IBM China Linux Technology Centre

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

[Peter Maydell]

On 22 November 2012 16:34, Li Zhang <zhlcindy@linux.vnet.ibm.com> wrote:
> On 2012年11月23日 00:18, Peter Maydell wrote:
>>
>> On 29 October 2012 10:25, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> From: zhlcindy@gmail.com <zhlcindy@gmail.com>
>>>
>>> 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.

-- PMM

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

[Li Zhang]

On 2012年11月23日 00:39, Peter Maydell wrote:
> On 22 November 2012 16:34, Li Zhang <zhlcindy@linux.vnet.ibm.com> wrote:
>> On 2012年11月23日 00:18, Peter Maydell wrote:
>>> On 29 October 2012 10:25, Alexander Graf <agraf@suse.de> wrote:
>>>> From: zhlcindy@gmail.com <zhlcindy@gmail.com>
>>>>
>>>> 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.
Yes, you are right, which cause this problem.
Do you think it's better to enabled USB even if there is no machine 
option in command line?
>
> -- PMM
>


-- 

Li Zhang
IBM China Linux Technology Centre

Re: [Qemu-devel] 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 <zhlcindy@linux.vnet.ibm.com> wrote:
>> On 2012年11月23日 00:18, Peter Maydell wrote:
>>> 
>>> On 29 October 2012 10:25, Alexander Graf <agraf@suse.de> wrote:
>>>> 
>>>> From: zhlcindy@gmail.com <zhlcindy@gmail.com>
>>>> 
>>>> 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

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

[Li Zhang]

On 2012年11月23日 00:44, Alexander Graf wrote:
> On 22.11.2012, at 17:39, Peter Maydell wrote:
>
>> On 22 November 2012 16:34, Li Zhang <zhlcindy@linux.vnet.ibm.com> wrote:
>>> On 2012年11月23日 00:18, Peter Maydell wrote:
>>>> On 29 October 2012 10:25, Alexander Graf <agraf@suse.de> wrote:
>>>>> From: zhlcindy@gmail.com <zhlcindy@gmail.com>
>>>>>
>>>>> 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.
OK. I will remove all the checks.
Sorry about that.
>
> Alex
>


-- 

Li Zhang
IBM China Linux Technology Centre

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

[Peter Maydell]

On 22 November 2012 16:39, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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.

Patch sent which fixes this bug.

-- PMM

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

[Li Zhang]

On 2012年11月23日 00:49, Peter Maydell wrote:
> On 22 November 2012 16:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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.
> Patch sent which fixes this bug.
Thanks for fixing that. :)
>
> -- PMM
>


-- 

Li Zhang
IBM China Linux Technology Centre