From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49558) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb0kp-0005E8-Ca for qemu-devel@nongnu.org; Tue, 20 Nov 2012 22:13:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tb0kn-0000BY-Bk for qemu-devel@nongnu.org; Tue, 20 Nov 2012 22:13:35 -0500 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:54091) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb0ka-0008P7-8p for qemu-devel@nongnu.org; Tue, 20 Nov 2012 22:13:33 -0500 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 21 Nov 2012 13:11:56 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qAL32OeY60162096 for ; Wed, 21 Nov 2012 14:02:25 +1100 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qAL3CtAJ005014 for ; Wed, 21 Nov 2012 14:12:55 +1100 Message-ID: <50AC469E.1060704@linux.vnet.ibm.com> Date: Wed, 21 Nov 2012 11:12:30 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1353404767-4495-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1353404767-4495-6-git-send-email-xiawenc@linux.vnet.ibm.com> <50AB5BC5.9030605@redhat.com> In-Reply-To: <50AB5BC5.9030605@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V10 5/7] libqblock type defines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, kraxel@redhat.com 于 2012-11-20 18:30, Paolo Bonzini 写道: > Il 20/11/2012 10:46, Wenchao Xia ha scritto: >> This patch contains type and macro defines used in APIs, one file for public >> usage, one for libqblock internal usage. > > Mostly looks good, just a few questions below. > >> Signed-off-by: Wenchao Xia >> --- >> libqblock/libqblock-internal.h | 75 ++++++++++++ >> libqblock/libqblock-types.h | 252 ++++++++++++++++++++++++++++++++++++++++ >> libqblock/libqblock.c | 6 - >> libqblock/libqblock.h | 1 - >> tests/check-libqblock-qcow2.c | 1 - >> 5 files changed, 327 insertions(+), 8 deletions(-) >> create mode 100644 libqblock/libqblock-internal.h >> >> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h >> new file mode 100644 >> index 0000000..75c0452 >> --- /dev/null >> +++ b/libqblock/libqblock-internal.h >> @@ -0,0 +1,75 @@ >> +/* >> + * QEMU block layer library >> + * >> + * Copyright IBM, Corp. 2012 >> + * >> + * Authors: >> + * Wenchao Xia >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +#ifndef LIBQBLOCK_INTERNAL >> +#define LIBQBLOCK_INTERNAL >> + >> +#include "glib.h" > > Use please. > OK. >> + >> +#include "block.h" >> +#include "block_int.h" > > Is block_int.h really needed? > Some type define in it was used before, let me check if it can be removed. >> +#include "libqblock-types.h" >> + >> +/* this file contains defines and types used inside the library. */ >> + >> +#define FUNC_FREE(p) g_free((p)) >> +#define FUNC_MALLOC(size) g_malloc((size)) >> +#define FUNC_CALLOC(nmemb, size) g_malloc0((nmemb)*(size)) >> +#define FUNC_STRDUP(p) g_strdup((p)) > > Why keep these? This macro make it easy to switch mem related functions if we change our mind in the future. >> + >> +#define CLEAN_FREE(p) { \ >> + FUNC_FREE(p); \ >> + (p) = NULL; \ >> +} > > What about calling it QB_FREE? > OK. >> + >> +/* details should be hidden to user */ >> +struct QBlockState { >> + BlockDriverState *bdrvs; >> + /* internal used file name now, if it is not NULL, it means >> + image was opened. >> + */ >> + char *filename; >> +}; >> + >> +struct QBlockContext { >> + /* last error */ >> + GError *g_error; >> + int err_ret; /* 1st level of error, the libqblock error number */ >> + int err_no; /* 2nd level of error, errno what below reports */ >> +}; >> + >> +/** >> + * QBlockStaticInfoAddr: a structure contains a set of pointer. >> + * >> + * this struct contains a set of pointer pointing to some >> + * property related to format or protocol. If a property is not available, >> + * it will be set as NULL. User could use this to get properties directly. >> + * >> + * @virt_size: virtual size, it is always not NULL. >> + * @backing_loc: backing file location. >> + * @encrypt: encryption flag. >> +*/ >> + >> +typedef struct QBlockStaticInfoAddr { >> + uint64_t *virt_size; >> + QBlockLocationInfo *backing_loc; >> + bool *encrypt; >> +} QBlockStaticInfoAddr; >> + >> +#define G_LIBQBLOCK_ERROR g_libqbock_error_quark() >> + >> +static inline GQuark g_libqbock_error_quark(void) > > libqblock, not libqbock. > my mistake. >> +{ >> + return g_quark_from_static_string("g-libqblock-error-quark"); >> +} >> +#endif >> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h >> index e69de29..77ad325 100644 >> --- a/libqblock/libqblock-types.h >> +++ b/libqblock/libqblock-types.h >> @@ -0,0 +1,252 @@ >> +/* >> + * QEMU block layer library >> + * >> + * Copyright IBM, Corp. 2012 >> + * >> + * Authors: >> + * Wenchao Xia >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2 or later. >> + * See the COPYING.LIB file in the top-level directory. >> + * >> + */ >> + >> +#ifndef LIBQBLOCK_TYPES_H >> +#define LIBQBLOCK_TYPES_H >> + >> +#include >> +#include >> +#include >> + >> +#if defined(__GNUC__) && __GNUC__ >= 4 >> + #ifdef LIBQB_BUILD >> + #define DLL_PUBLIC __attribute__((visibility("default"))) >> + #else >> + #define DLL_PUBLIC >> + #endif >> +#endif >> + >> +/* this library is designed around this core struct. */ >> +typedef struct QBlockState QBlockState; >> + >> +/* every thread should have a context. */ >> +typedef struct QBlockContext QBlockContext; >> + >> +/* flag used in open and create */ >> +#define LIBQBLOCK_O_RDWR 0x0002 >> +/* do not use the host page cache */ >> +#define LIBQBLOCK_O_NOCACHE 0x0020 >> +/* use write-back caching */ >> +#define LIBQBLOCK_O_CACHE_WB 0x0040 >> +/* don't open the backing file */ >> +#define LIBQBLOCK_O_NO_BACKING 0x0100 >> +/* disable flushing on this disk */ >> +#define LIBQBLOCK_O_NO_FLUSH 0x0200 >> + >> +#define LIBQBLOCK_O_CACHE_MASK \ >> + (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH) >> + >> +#define LIBQBLOCK_O_VALID_MASK \ >> + (LIBQBLOCK_O_RDWR | LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | \ >> + LIBQBLOCK_O_NO_BACKING | LIBQBLOCK_O_NO_FLUSH) >> + >> +typedef enum QBlockProtocol { >> + QB_PROTO_NONE = 0, >> + QB_PROTO_FILE, >> + QB_PROTO_MAX >> +} QBlockProtocol; >> + >> +typedef struct QBlockProtocolOptionsFile { >> + const char *filename; >> +} QBlockProtocolOptionsFile; >> + >> +/** >> + * struct QBlockLocationInfo: contains information about how to find the image >> + * >> + * @prot_type: protocol type, now only support FILE. >> + * @o_file: file protocol related attributes. >> + * @reserved: reserved bytes for ABI. >> + */ >> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512) >> +typedef struct QBlockLocationInfo { >> + QBlockProtocol prot_type; >> + union { >> + QBlockProtocolOptionsFile o_file; >> + uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE/8]; >> + }; >> +} QBlockLocationInfo; >> + >> + >> +/* format related options */ >> +typedef enum QBlockFormat { >> + QB_FMT_NONE = 0, >> + QB_FMT_COW, >> + QB_FMT_QED, >> + QB_FMT_QCOW, >> + QB_FMT_QCOW2, >> + QB_FMT_RAW, >> + QB_FMT_RBD, >> + QB_FMT_SHEEPDOG, >> + QB_FMT_VDI, >> + QB_FMT_VMDK, >> + QB_FMT_VPC, >> + QB_FMT_MAX >> +} QBlockFormat; >> + >> +typedef struct QBlockFormatOptionsCOW { >> + uint64_t virt_size; >> + QBlockLocationInfo backing_loc; >> +} QBlockFormatOptionsCOW; >> + >> +typedef struct QBlockFormatOptionsQED { >> + uint64_t virt_size; >> + QBlockLocationInfo backing_loc; >> + QBlockFormat backing_fmt; >> + uint64_t cluster_size; /* unit is bytes */ >> + uint64_t table_size; /* unit is clusters */ >> +} QBlockFormatOptionsQED; >> + >> +typedef struct QBlockFormatOptionsQCOW { >> + uint64_t virt_size; >> + QBlockLocationInfo backing_loc; >> + bool encrypt; >> +} QBlockFormatOptionsQCOW; >> + >> +/* "Compatibility level (0.10 or 1.1)" */ >> +typedef enum QBlockFormatOptionsQCOW2CompatLv { >> + QB_FMT_QCOW2_COMPAT_DEFAULT = 0, >> + QB_FMT_QCOW2_COMPAT_V0_10, >> + QB_FMT_QCOW2_COMPAT_V1_10, >> +} QBlockFormatOptionsQCOW2CompatLv; >> + >> +/* off or metadata */ >> +typedef enum QBlockFormatOptionsQCOW2PreAlloc { >> + QB_FMT_QCOW2_PREALLOC_DEFAULT = 0, >> + QB_FMT_QCOW2_PREALLOC_OFF, >> + QB_FMT_QCOW2_PREALLOC_METADATA, >> +} QBlockFormatOptionsQCOW2PreAlloc; >> + >> +typedef struct QBlockFormatOptionsQCOW2 { >> + uint64_t virt_size; >> + QBlockLocationInfo backing_loc; >> + QBlockFormat backing_fmt; >> + bool encrypt; >> + uint64_t cluster_size; /* unit is bytes */ >> + QBlockFormatOptionsQCOW2CompatLv cpt_lv; >> + QBlockFormatOptionsQCOW2PreAlloc pre_mode; >> +} QBlockFormatOptionsQCOW2; >> + >> +typedef struct QBlockFormatOptionsRAW { >> + uint64_t virt_size; >> +} QBlockFormatOptionsRAW; >> + >> +typedef struct QBlockFormatOptionsRBD { >> + uint64_t virt_size; >> + uint64_t cluster_size; >> +} QBlockFormatOptionsRBD; >> + >> +/* off or full */ >> +typedef enum QBlockFormatOptionsSDPreAlloc { >> + QB_FMT_SD_PREALLOC_DEFAULT = 0, >> + QB_FMT_SD_PREALLOC_OFF, >> + QB_FMT_SD_PREALLOC_FULL, >> +} QBlockFormatOptionsSDPreAlloc; >> + >> +typedef struct QBlockFormatOptionsSD { >> + uint64_t virt_size; >> + QBlockLocationInfo backing_loc; >> + QBlockFormatOptionsSDPreAlloc pre_mode; >> +} QBlockFormatOptionsSD; >> + >> +typedef enum QBlockFormatOptionsVDIPreAlloc { >> + QB_FMT_VDI_PREALLOC_DEFAULT = 0, >> + QB_FMT_VDI_PREALLOC_OFF, >> + QB_FMT_VDI_PREALLOC_METADATA, >> +} QBlockFormatOptionsVDIPreAlloc; >> + >> +typedef struct QBlockFormatOptionsVDI { >> + uint64_t virt_size; >> + uint64_t cluster_size; >> + QBlockFormatOptionsVDIPreAlloc pre_mode; >> +} QBlockFormatOptionsVDI; >> + >> +/* whether compact to vmdk verion 6 */ >> +typedef enum QBlockFormatOptionsVMDKCompatLv { >> + QB_FMT_VMDK_COMPAT_DEFAULT = 0, >> + QB_FMT_VMDK_COMPAT_VMDKV6_FALSE, >> + QB_FMT_VMDK_COMPAT_VMDKV6_TRUE, >> +} QBlockFormatOptionsVMDKCompatLv; >> + >> +/* vmdk flat extent format, values: >> +"{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | >> +twoGbMaxExtentFlat | streamOptimized} */ >> +typedef enum QBlockFormatOptionsVMDKSubfmt { >> + QB_FMT_VMDK_SUBFMT_DEFAULT = 0, >> + QB_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE, >> + QB_FMT_VMDK_SUBFMT_MONOLITHIC_FLAT, >> + QB_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_SPARSE, >> + QB_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_FLAT, >> + QB_FMT_VMDK_SUBFMT_STREAM_OPTIMIZED, >> +} QBlockFormatOptionsVMDKSubfmt; >> + >> +typedef struct QBlockFormatOptionsVMDK { >> + uint64_t virt_size; >> + QBlockLocationInfo backing_loc; >> + QBlockFormatOptionsVMDKCompatLv cpt_lv; >> + QBlockFormatOptionsVMDKSubfmt subfmt; >> +} QBlockFormatOptionsVMDK; >> + >> +/* "{dynamic (default) | fixed} " */ >> +typedef enum QBlockFormatOptionsVPCSubfmt { >> + QB_FMT_VPC_SUBFMT_DEFAULT = 0, >> + QB_FMT_VPC_SUBFMT_DYNAMIC, >> + QB_FMT_VPC_SUBFMT_FIXED, >> +} QBlockFormatOptionsVPCSubfmt; >> + >> +typedef struct QBlockFormatOptionsVPC { >> + uint64_t virt_size; >> + QBlockFormatOptionsVPCSubfmt subfmt; >> +} QBlockFormatOptionsVPC; >> + >> +#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2) > > Why? QBlockFormatInfo contains QBlockLocationInfo for backing file in some formats, so make this structure twice size as QBlockLocationInfo. > >> + >> +/** >> + * struct QBlockFormatInfo: contains information about how to retrieve data >> + * from the image >> + * >> + * @fmt_type: format type. >> + * @o_cow~@o_vdi: format related attributes. >> + * @reserved: reserved bytes for ABI. >> + */ >> +typedef struct QBlockFormatInfo { >> + QBlockFormat fmt_type; >> + union { >> + QBlockFormatOptionsCOW o_cow; >> + QBlockFormatOptionsQED o_qed; >> + QBlockFormatOptionsQCOW o_qcow; >> + QBlockFormatOptionsQCOW2 o_qcow2; >> + QBlockFormatOptionsRAW o_raw; >> + QBlockFormatOptionsRBD o_rbd; >> + QBlockFormatOptionsSD o_sd; >> + QBlockFormatOptionsVDI o_vdi; >> + QBlockFormatOptionsVMDK o_vmdk; >> + QBlockFormatOptionsVPC o_vpc; >> + uint64_t reserved[QBLOCK_FMT_OPTIONS_UNION_SIZE/8]; >> + }; >> +} QBlockFormatInfo; >> + >> +/** >> + * QBlockStaticInfo: information about the block image. >> + * >> + * @loc: location information. >> + * @fmt: format information. >> + * @sector_size: how many bytes in a sector, it is 512 usually. >> + */ >> +typedef struct QBlockStaticInfo { >> + QBlockLocationInfo loc; >> + QBlockFormatInfo fmt; >> + int sector_size; >> +} QBlockStaticInfo; >> + >> + >> +#endif >> diff --git a/libqblock/libqblock.c b/libqblock/libqblock.c >> index 4c18c41..e69de29 100644 >> --- a/libqblock/libqblock.c >> +++ b/libqblock/libqblock.c >> @@ -1,6 +0,0 @@ >> -#include "libqblock.h" >> - >> -int libqb_test(void) >> -{ >> - return 0; >> -} > > Why? > This function is used in build system patch to verify final link is OK, it should be removed in next API patch, my mistake. >> diff --git a/libqblock/libqblock.h b/libqblock/libqblock.h >> index b0f9daf..e69de29 100644 >> --- a/libqblock/libqblock.h >> +++ b/libqblock/libqblock.h >> @@ -1 +0,0 @@ >> -__attribute__((visibility("default"))) int libqb_test(void); >> diff --git a/tests/check-libqblock-qcow2.c b/tests/check-libqblock-qcow2.c >> index 50a4df3..6f5ccac 100644 >> --- a/tests/check-libqblock-qcow2.c >> +++ b/tests/check-libqblock-qcow2.c >> @@ -1,6 +1,5 @@ >> #include "libqblock.h" >> int main(int argc, char **argv) >> { >> - libqb_test(); > > Why? > >> return 0; >> } >> > -- Best Regards Wenchao Xia