From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([140.186.70.92]:35617)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1RR2gR-0006Q8-2P
	for qemu-devel@nongnu.org; Thu, 17 Nov 2011 09:11:19 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1RR2gL-0007KO-FE
	for qemu-devel@nongnu.org; Thu, 17 Nov 2011 09:11:19 -0500
Received: from mail-ww0-f53.google.com ([74.125.82.53]:65051)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1RR2gL-0007KB-Am
	for qemu-devel@nongnu.org; Thu, 17 Nov 2011 09:11:13 -0500
Received: by wwf27 with SMTP id 27so2680070wwf.10
	for <qemu-devel@nongnu.org>; Thu, 17 Nov 2011 06:11:12 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <4EC4991C.1070306@linux.vnet.ibm.com>
References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com>
	<20111111064802.15024.83662.sendpatchset@skannery.in.ibm.com>
	<CAJSP0QU=HELGj0bPotqVSADNKEAXT5=kLXiQ_45zWRppn3SbnA@mail.gmail.com>
	<4EC4991C.1070306@linux.vnet.ibm.com>
Date: Thu, 17 Nov 2011 14:11:12 +0000
Message-ID: <CAJSP0QWZaCt2V-RGk_Ekomsw9XmgwJrawNU6RSQ3227Q2OrkrA@mail.gmail.com>
From: Stefan Hajnoczi <stefanha@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option
	'hostcache'
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: supriyak@linux.vnet.ibm.com
Cc: Kevin Wolf <kwolf@redhat.com>, Luiz Capitulino <lcapitulino@redhat.com>, Christoph Hellwig <hch@lst.de>, qemu-devel@nongnu.org

On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
<supriyak@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
>> <supriyak@linux.vnet.ibm.com> =A0wrote:
>>>
>>> + =A0 =A0 =A0 =A0if ((hostcache =3D qemu_opt_get_bool(opts, "hostcache"=
, -1)) !=3D
>>> -1) {
>>
>> This does not work. =A0qemu_opt_get_bool() takes a bool default argument
>> and returns a bool. =A0(bool)-1 =3D=3D true. =A0But (int)true =3D=3D 1 a=
nd you
>> cannot expect it to ever equal -1.
>>
>> Try this:
>>
>> if (qemu_opt_get(opts, "hostcache")&&
>> =A0 =A0 !qemu_opt_get_bool(opts, "hostcache", false)) {
>> =A0 =A0 bdrv_flags |=3D BDRV_O_NOCACHE;
>> }
>>
>> Stefan
>>
>
> Thanks! for pointing this.
> Does the following look ok?
>
> =A0if ((hostcache =3D qemu_opt_get_bool(opts, "hostcache", 1) =3D=3D 0) {
> =A0 =A0 bdrv_flags |=3D BDRV_O_NOCACHE;
> =A0}
>
> 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=3D and hostcache=3D may not be used together.
2. cache=3D sets bdrv_flags.
3. hostcache=3D may |=3D BDRV_O_NOCACHE.
4. No option defaults to cache=3Dwritethrough (bdrv_flags &=3D ~BDRV_O_CACH=
E_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=3D 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.

Stefan