Re: [Qemu-devel] [V2 2/2]Qemu: Add commands "hostcache_set" and "hostcache_get"

[Supriya Kannery]

On 05/20/2011 01:50 PM, Stefan Hajnoczi wrote:
> On Thu, May 19, 2011 at 10:38:03PM +0530, Supriya Kannery wrote:
> > Monitor commands "hostcache_set" and "hostcache_get" added for dynamic
> > host cache change and display of host cache setting respectively.
>
> A generic command for changing block device options would be nice,
> althought I don't see other options where it makes sense to change them
> at runtime.
>
> The alternative would be:
>
> block_set hostcache on
>
> "block_set", {"device": "ide1-cd0", "name": "hostcache", "enable": true}
>
> The hostcache_get information would be part of query-block output:
>           {
>              "device":"ide0-hd0",
>              "locked":false,
>              "removable":false,
>              "inserted":{
>                 "ro":false,
>                 "drv":"qcow2",
>                 "encrypted":false,
>                 "file":"disks/test.img"
>                "hostcache":true,
>              },
>              "type":"hd"
>           },
>
> This approach is extensible if more options need to be exposed.

Sure, I will resubmit this patchset, after making this feature more 
generic. Can you pls help finding atleast one or two options (other than 
hostcache) which can be changed dynamically. This will help me evaluate 
the generic approach.

> > +        .params     = "device",
> > +        .help       = "retrieve host cache settings for device",
>
> Please make it clear these operations affect block devices:
> "for block device"

ok

> > +    /*
> > +     * A failed attempt to reopen the image file must lead to 'abort()'
> > +     */
> > +    if (ret != 0) {
> > +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> > +        abort();
>
> The error is never reported on a QMP monitor because qerror_report()
> simply stashes away the qerror.  The QMP client doesn't have a chance to
> read the error before QEMU terminates.

Verified this from qemu command line and the error got displayed before 
aborting (when the image file was renamed while VM was running).

> > +
> > +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
>
> Consistently using "hostcache" or "host_cache" would be nice.

ok

> > +        return -1;
>
> This shouldn't be a failure and please don't use -1 when a negative
> errno indicates failure.  -1 == -EPERM.  The return value should be 0
> here.
>
> > +    }
>
> Anyway, this whole check is unnecessary since bdrv_reopen() already
> performs it.

will take this off

> Please run scripts/checkpatch.pl before submitting patches.

ok..will do

> > +
> > +Arguments:
> > +
> > +- "device": the device's ID, must be unique (json-string)
> > +- "cache": host cache value "off" or "on" (json-string)
>
> There is a boolean value that can be used instead of string on|off.  See
> the set_link command.

ok

> Stefan