From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37041) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiLh0-00004x-DY for qemu-devel@nongnu.org; Tue, 11 Dec 2012 04:00:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TiLgt-0007bH-6n for qemu-devel@nongnu.org; Tue, 11 Dec 2012 03:59:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:12150) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiLgs-0007b0-Sx for qemu-devel@nongnu.org; Tue, 11 Dec 2012 03:59:51 -0500 Message-ID: <50C6F5FF.7080900@redhat.com> Date: Tue, 11 Dec 2012 09:59:43 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1354776711-12449-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1354776711-12449-5-git-send-email-wdongxu@linux.vnet.ibm.com> <50C617B6.2050200@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V17 4/6] rename qcow2-cache.c to block-cache.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Xu Wang Cc: qemu-devel Am 11.12.2012 09:25, schrieb Dong Xu Wang: > On Tue, Dec 11, 2012 at 1:11 AM, Kevin Wolf 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 >>> 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 >>> + * >>> + * 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 >>> + * >>> + * 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