qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: supriya kannery
Subject: Re: [Qemu-devel] [v9 Patch 4/6]Qemu: Add commandline -drive option 'hostcache'
Date: Tue, 22 Nov 2011 13:40:53 +0530
User-agent: Thunderbird 2.0.0.14 (X11/20080501)

Stefan Hajnoczi wrote:
On Mon, Nov 21, 2011 at 12:28 PM, supriya kannery <address@hidden> wrote:
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= ]

Okay, I can see that line of reasoning but then hostcache= does not
provide much value on the command-line.  Perhaps it's better to drop
this patch and not merge a new -drive option until the guest-visible
write cache enable support is also merged?


Let us have the implementation for hostcache= as command line option, with
the condition that if both cache= and hostcache= are specified together,
then depending upon enable/disable value specified for hostcache, corresponding bit in cache flags can be set/reset. That way, there is a value add on specifying hostcache in cmdline as well as user will be able to control hostcache value from cmdline
itself.

The interesting feature that this series adds is changing of hostcache
at run-time.


Yes.. will resume with dynamic hostcache change part  to make it
usable by qemu

- thanks, Supriya
Stefan





reply via email to

[Prev in Thread] Current Thread [Next in Thread]