Re: [Qemu-devel] [PATCH V17 4/6] rename qcow2-cache.c to block-cache.c

[Kevin Wolf <kwolf@redhat.com>]

Am 11.12.2012 09:25, schrieb Dong Xu Wang:
> On Tue, Dec 11, 2012 at 1:11 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 06.12.2012 07:51, schrieb Dong Xu Wang:
>>> We will re-use qcow2-cache as block layer common cache code,
>>> so change its name and made some changes, define a struct named
>>> BlockTableType, pass BlockTableType and table size parameters to
>>> block cache initialization function.
>>>
>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>

>>> diff --git a/block/block-cache.h b/block/block-cache.h
>>> new file mode 100644
>>> index 0000000..4efa06e
>>> --- /dev/null
>>> +++ b/block/block-cache.h
>>> @@ -0,0 +1,76 @@
>>> +/*
>>> + * QEMU Block Layer Cache
>>> + *
>>> + * Copyright IBM, Corp. 2012
>>> + *
>>> + * Authors:
>>> + *  Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>> + *
>>> + * This file is based on qcow2-cache.c, see its copyrights below:
>>> + *
>>> + * L2/refcount table cache for the QCOW2 format
>>> + *
>>> + * Copyright (c) 2010 Kevin Wolf <kwolf@redhat.com>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>> + * of this software and associated documentation files (the "Software"), to deal
>>> + * in the Software without restriction, including without limitation the rights
>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>> + * copies of the Software, and to permit persons to whom the Software is
>>> + * furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice shall be included in
>>> + * all copies or substantial portions of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>> + * THE SOFTWARE.
>>> + */
>>> +
>>> +#ifndef BLOCK_CACHE_H
>>> +#define BLOCK_CACHE_H
>>> +
>>> +typedef enum {
>>> +    BLOCK_TABLE_REF,
>>> +    BLOCK_TABLE_L2,
>>> +} BlockTableType;
>>
>> I'm not excited about exposing these types, but it's okay for now. We
>> can always change the implementation later to be nicer.
>>
>>> +
>>> +typedef struct BlockCachedTable {
>>> +    void    *table;
>>> +    int64_t offset;
>>> +    bool    dirty;
>>> +    int     cache_hits;
>>> +    int     ref;
>>> +} BlockCachedTable;
>>> +
>>> +struct BlockCache {
>>> +    BlockCachedTable    *entries;
>>> +    struct BlockCache   *depends;
>>> +    int                 size;
>>> +    size_t              cluster_size;
>>> +    BlockTableType      table_type;
>>> +    bool                depends_on_flush;
>>> +};
>>
>> Why have these definitions been moved to the header file? They are
>> supposed to be private to the implementation.
>>
> Both qcow2.h:BDRVQcowState and add-cow.c: BDRVAddCowState have a
> member typed  BlockCache,
> such as:
> typedef struct BDRVAddCowState {
>     BlockDriverState    *image_hd;
>     CoMutex             lock;
>     int                 cluster_size;
>     BlockCache          *bitmap_cache;
>     uint64_t            bitmap_size;
>     AddCowHeader        header;
>     char                backing_fmt[16];
>     char                image_fmt[16];
> } BDRVAddCowState;
> 
> So I have to move the definitions to the header file, so that both
> add-cow.c and qcow2.h can use  BlockCache.
> Is it Okay?

The fields are actually of the type BlockCache*, i.e. a pointer. They
don't need the full definition of the struct, a forward declaration is
sufficient. So it's enough to have these lines from qcow2.h in the header:

struct Qcow2Cache;
typedef struct Qcow2Cache Qcow2Cache;

Kevin