On Mon, Jul 25, 2011 at 1:52 PM, Supriya Kannery <supri...@in.ibm.com> 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
