Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'

[supriya kannery]

Stefan Hajnoczi wrote:
> On Thu, Nov 17, 2011 at 5:18 AM, Supriya Kannery
> <address@hidden> wrote:
>   
> > On 11/17/2011 01:36 AM, Stefan Hajnoczi wrote:
> >     
> > > On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery
> > > <address@hidden>  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
>
>