On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery <supri...@in.ibm.com> wrote:
> Stefan Hajnoczi wrote:
>>
>> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
>> <supri...@linux.vnet.ibm.com> wrote:
>>
>>>
>>> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
>>>
>>>>
>>>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
>>>> <supri...@linux.vnet.ibm.com> wrote:
>>>>
>>>>>
>>>>> + if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) !=
>>>>> -1) {
>>>>>
>>>>
>>>> This does not work. qemu_opt_get_bool() takes a bool default argument
>>>> and returns a bool. (bool)-1 == true. But (int)true == 1 and you
>>>> cannot expect it to ever equal -1.
>>>>
>>>> Try this:
>>>>
>>>> if (qemu_opt_get(opts, "hostcache")&&
>>>> !qemu_opt_get_bool(opts, "hostcache", false)) {
>>>> bdrv_flags |= BDRV_O_NOCACHE;
>>>> }
>>>>
>>>> Stefan
>>>>
>>>>
>>>
>>> Thanks! for pointing this.
>>> Does the following look ok?
>>>
>>> if ((hostcache = qemu_opt_get_bool(opts, "hostcache", 1) == 0) {
>>> bdrv_flags |= BDRV_O_NOCACHE;
>>> }
>>>
>>> If either "hostcache" is not at all specified or it is specified
>>> as "on", qemu_opt_get_bool will return 1, which can be ignored
>>> as bdrv_flags is initialized to 0.
>>>
>>
>> It depends on the overall way this should work. I think this captures
>> all the cases:
>>
>> 1. cache= and hostcache= may not be used together.
>> 2. cache= sets bdrv_flags.
>> 3. hostcache= may |= BDRV_O_NOCACHE.
>> 4. No option defaults to cache=writethrough (bdrv_flags &=
>> ~BDRV_O_CACHE_MASK).
>>
>> The code you posted will work although I find it a bit weird how it
>> also includes case #4. IMO it's cleanest to just do case #3 by
>> testing whether or not the hostcache= option is set.
>>
>> BTW, is there a check for case #1 in your patch series. I thought I
>> saw one earlier but now I can't find it.
>>
>
> Following is what I tried to accomplish:
>
> 1. cache= and hostcache= may be used together. Cache= will override
> hostcache= if specified together
> This condition can be retained till cache-xx can be completely replaced by
> combinations of separate cmdline options defined for setting hostcache,
> flush, and WCE
> [I think I can change the current implementation to have Cache=xx ORed with
> hostcache=yy if they are specified together instead of just ignoring
> hostcache= ]
Okay, I can see that line of reasoning but then hostcache= does not
provide much value on the command-line. Perhaps it's better to drop
this patch and not merge a new -drive option until the guest-visible
write cache enable support is also merged?
The interesting feature that this series adds is changing of hostcache
at run-time.
Stefan
