From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH v2] introduce a cache options for PV disks
Date: Thu, 27 Jun 2013 18:50:00 +0100
Message-ID: <alpine.DEB.2.02.1306271848210.4782@kaball.uk.xensource.com>
References: <alpine.DEB.2.02.1306271238100.4782@kaball.uk.xensource.com>
	<20940.26172.875919.504123@mariner.uk.xensource.com>
	<alpine.DEB.2.02.1306271800360.4782@kaball.uk.xensource.com>
	<20940.31210.191976.244639@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20940.31210.191976.244639@mariner.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: xen-devel@lists.xensource.com, Ian Campbell <Ian.Campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Thu, 27 Jun 2013, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [PATCH v2] introduce a cache options for PV disks"):
> > It's mostly OK, just few minor corrections for code readability
> 
> Thanks.
> 
> > >      int info = 0;
> > > +    char *directiosafe == NULL;
> >                            ^ =
> 
> I did warn that I hadn't compiled it :-).
> 
> > >  out_error:
> > > @@ -773,6 +779,7 @@ out_error:
> > >      blkdev->dev = NULL;
> > >      g_free(blkdev->devtype);
> > >      blkdev->devtype = NULL;
> > > +    g_free(directiosafe);
> > 
> > maybe add
> > 
> > blkdev->directiosafe = false;
> 
> Sure, does no harm.
> 
> > > +    if (blkdev->directiosafe) {
> > > +        qflags = BDRV_O_NOCACHE | BDRV_O_NATIVE_AIO;
> > > +    }
> > 
> > Please change this into:
> > 
> > if (blkdev->directiosafe) {
> >     qflags = BDRV_O_NOCACHE | BDRV_O_NATIVE_AIO;
> > } else {
> >     qflags = BDRV_O_CACHE_WB;
> > }
> 
> I don't think that can be right.  The result would be that the value
> non-directiosafe value of qflags is changed.  But I will break it
> apart as you suggest.

BDRV_O_NATIVE_AIO is ignored if BDRV_O_NOCACHE is not also set, so there
would be not behavioural change. However the code would be clearer.