Re: [Qemu-devel] [v2 Patch 5/9]block: qcow2 image file reopen

[Kevin Wolf <kwolf@redhat.com>]

Am 09.08.2012 06:26, schrieb Jeff Cody:
> On 07/30/2012 05:35 PM, Supriya Kannery wrote:
>> qcow2 driver changes for bdrv_reopen_xx functions to
>> safely reopen image files. Reopening of image files while
>> changing hostcache dynamically is handled here.
>>
>> Signed-off-by: Supriya Kannery <supriyak@linux.vnet.ibm.com>
>>
>> ---
>> Index: qemu/block/qcow2.c
>> ===================================================================
>> --- qemu.orig/block/qcow2.c
>> +++ qemu/block/qcow2.c
>> @@ -52,10 +52,19 @@ typedef struct {
>>      uint32_t magic;
>>      uint32_t len;
>>  } QCowExtension;
>> +
>> +typedef struct BDRVQcowReopenState {
>> +    BDRVReopenState reopen_state;
>> +    BDRVQcowState *stash_s;
>> +} BDRVQcowReopenState;
>> +
>>  #define  QCOW2_EXT_MAGIC_END 0
>>  #define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
>>  #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
>>  
>> +static void qcow2_stash_state(BDRVQcowState *stashed_state, BDRVQcowState *s);
>> +static void qcow2_revert_state(BDRVQcowState *s, BDRVQcowState *stashed_state);
>> +
>>  static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
>>  {
>>      const QCowHeader *cow_header = (const void *)buf;
>> @@ -434,6 +443,169 @@ static int qcow2_open(BlockDriverState *
>>      return ret;
>>  }
>>  
>> +static int qcow2_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs,
>> +                               int flags)
>> +{
>> +    BDRVQcowReopenState *qcow2_rs = g_malloc0(sizeof(BDRVQcowReopenState));
>> +    int ret = 0;
>> +    BDRVQcowState *s = bs->opaque;
>> +
>> +    qcow2_rs->reopen_state.bs = bs;
>> +
>> +    /* save state before reopen */
>> +    qcow2_rs->stash_s = g_malloc0(sizeof(BDRVQcowState));
>> +    qcow2_stash_state(qcow2_rs->stash_s, s);
>> +    *prs = &(qcow2_rs->reopen_state);
>> +
>> +    /* Reopen file with new flags */
>> +     ret = qcow2_open(bs, flags);
>> +     return ret;
>> +}
>> +
>> +static void qcow2_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs)
>> +{
>> +    BDRVQcowReopenState *qcow2_rs;
>> +    BDRVQcowState *stashed_s;
>> +
>> +    qcow2_rs = container_of(rs, BDRVQcowReopenState, reopen_state);
>> +    stashed_s = qcow2_rs->stash_s;
>> +
>> +    qcow2_cache_flush(bs, stashed_s->l2_table_cache);
>> +    qcow2_cache_flush(bs, stashed_s->refcount_block_cache);
>> +
>> +    qcow2_cache_destroy(bs, stashed_s->l2_table_cache);
>> +    qcow2_cache_destroy(bs, stashed_s->refcount_block_cache);
>> +
>> +    g_free(stashed_s->unknown_header_fields);
>> +    cleanup_unknown_header_ext(bs);
>> +
>> +    g_free(stashed_s->cluster_cache);
>> +    qemu_vfree(stashed_s->cluster_data);
> 
> 
> 
>> +    qcow2_refcount_close(bs);
>> +    qcow2_free_snapshots(bs);
> 
> I assume these are unintentional?  I believe this is what is causing your
> double-free issue.

This whole patch looks a bit complicated and brittle. What qcow2 state
do you actually expect to change with a new qcow2_open()? Is it even
required to do more than just modifying bs->file? qcow2 doesn't really
do anything with the flags.

Kevin