* Re: [Qemu-devel] commit 094b287 "Add USB option in machine options" broke i386 -usb
2012-11-22 16:39 ` Peter Maydell
@ 2012-11-22 16:43 ` Li Zhang
2012-11-22 16:44 ` Alexander Graf
2012-11-22 16:49 ` Peter Maydell
2 siblings, 0 replies; 8+ messages in thread
From: Li Zhang @ 2012-11-22 16:43 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, Alexander Graf, qemu-devel qemu-devel,
Blue Swirl, qemu-ppc@nongnu.org List, zhlcindy@gmail.com,
Aurelien Jarno
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] commit 094b287 "Add USB option in machine options" broke i386 -usb
2012-11-22 16:39 ` Peter Maydell
2012-11-22 16:43 ` Li Zhang
@ 2012-11-22 16:44 ` Alexander Graf
2012-11-22 16:48 ` Li Zhang
2012-11-22 16:49 ` Peter Maydell
2 siblings, 1 reply; 8+ messages in thread
From: Alexander Graf @ 2012-11-22 16:44 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, qemu-devel qemu-devel, Li Zhang, Blue Swirl,
qemu-ppc@nongnu.org List, zhlcindy@gmail.com, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] commit 094b287 "Add USB option in machine options" broke i386 -usb
2012-11-22 16:44 ` Alexander Graf
@ 2012-11-22 16:48 ` Li Zhang
0 siblings, 0 replies; 8+ messages in thread
From: Li Zhang @ 2012-11-22 16:48 UTC (permalink / raw)
To: Alexander Graf
Cc: Peter Maydell, Anthony Liguori, qemu-devel qemu-devel, Blue Swirl,
qemu-ppc@nongnu.org List, zhlcindy@gmail.com, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] commit 094b287 "Add USB option in machine options" broke i386 -usb
2012-11-22 16:39 ` Peter Maydell
2012-11-22 16:43 ` Li Zhang
2012-11-22 16:44 ` Alexander Graf
@ 2012-11-22 16:49 ` Peter Maydell
2012-11-22 16:50 ` Li Zhang
2 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2012-11-22 16:49 UTC (permalink / raw)
To: Li Zhang
Cc: Anthony Liguori, Alexander Graf, qemu-devel qemu-devel,
Blue Swirl, qemu-ppc@nongnu.org List, zhlcindy@gmail.com,
Aurelien Jarno
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] commit 094b287 "Add USB option in machine options" broke i386 -usb
2012-11-22 16:49 ` Peter Maydell
@ 2012-11-22 16:50 ` Li Zhang
0 siblings, 0 replies; 8+ messages in thread
From: Li Zhang @ 2012-11-22 16:50 UTC (permalink / raw)
To: Peter Maydell
Cc: Anthony Liguori, Alexander Graf, qemu-devel qemu-devel,
Blue Swirl, qemu-ppc@nongnu.org List, zhlcindy@gmail.com,
Aurelien Jarno
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
^ permalink raw reply [flat|nested] 8+ messages in thread