From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:33407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSTJq-0000PB-6M for qemu-devel@nongnu.org; Mon, 21 Nov 2011 07:49:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RSTJk-0007ly-C7 for qemu-devel@nongnu.org; Mon, 21 Nov 2011 07:49:54 -0500 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:50455) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RSTJj-0007lf-Nt for qemu-devel@nongnu.org; Mon, 21 Nov 2011 07:49:48 -0500 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 21 Nov 2011 18:19:41 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pALCnZM82687008 for ; Mon, 21 Nov 2011 18:19:35 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pALCnZdN009888 for ; Mon, 21 Nov 2011 18:19:35 +0530 Message-ID: <4ECA43F2.7080506@in.ibm.com> Date: Mon, 21 Nov 2011 17:58:34 +0530 From: supriya kannery MIME-Version: 1.0 References: <20111111064707.15024.69847.sendpatchset@skannery.in.ibm.com> <20111111064802.15024.83662.sendpatchset@skannery.in.ibm.com> <4EC4991C.1070306@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , supriyak@linux.vnet.ibm.com, Luiz Capitulino , Christoph Hellwig , qemu-devel@nongnu.org Stefan Hajnoczi wrote: > On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery > wrote: > >> On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote: >> >>> On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery >>> 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= ] 2. If only cache= specified sets bdrv_flags. 3. If only hostcache= specified, bdrv_flags |= BDRV_O_NOCACHE 4. No option defaults to cache=writethrough (bdrv_flags &= ~BDRV_O_CACHE_MASK). So the check I had earlier for case #1 in your list, I changed that to allow both the options to co-exist. -thanks, Supriya > Stefan > >