Re: [PATCH v2] introduce a cache options for PV disks

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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.