Re: [Qemu-devel] [V4 Patch 3/4 - Updated]Qemu: Command "block_set" for dynamic block params change

[Stefan Hajnoczi]

On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery <address@hidden> wrote:
> On 07/25/2011 12:00 PM, Stefan Hajnoczi wrote:
>>
>> On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
>>> +    if (bdrv_is_inserted(bs)) {
>>> +        /* Reopen file with changed set of flags */
>>> +        return bdrv_reopen(bs, bdrv_flags);
>>> +    } else {
>>> +        /* Save hostcache change for future use */
>>> +        bs->open_flags = bdrv_flags;
>
>>
>> Can you explain the scenario where this works?
>>
>> Looking at do_change_block() the flags will be clobbered so saving them
>> away does not help.
>
> Intention here is to use the changed hostcache setting during device
> insertion and there after. To apply this to 'change' command (during
> insertion of a device), will need to make the following code changes
> in do_change_block.
>
> +
> +    bdrv_flags = bs->open_flags;
> +    bdrv_flags |= bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>     bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
>     if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
>         qerror_report(QERR_OPEN_FILE_FAILED, filename);
>
> Need to track bs->open_flags a bit more to see what other changes
> needed to ensure usage of changed hostcache setting until user
> initiates next hostcache change explicitly for that drive.
>
> Please suggest... should we be saving the hostcache change for
> future use or just inhibit user from changing hostcache setting
> for an empty drive?

The 'change' command does not support any cache= options today.  It
always opens the new image with cache=writethrough semantics.

This seems like a bug in 'change' to me.  We should preserve cache=
settings across change or at least provide a way to specify them as
arguments.

Perhaps your existing code is fine.  When 'change' is fixed or
replaced then 'block_set' will work across 'change' too.

Kevin: Thoughts?

Stefan