From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34102) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THAmE-0005rk-S1 for qemu-devel@nongnu.org; Thu, 27 Sep 2012 05:53:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1THAm9-0004lY-B0 for qemu-devel@nongnu.org; Thu, 27 Sep 2012 05:53:02 -0400 Received: from e28smtp08.in.ibm.com ([122.248.162.8]:58516) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1THAm8-0004lH-5z for qemu-devel@nongnu.org; Thu, 27 Sep 2012 05:52:57 -0400 Received: from /spool/local by e28smtp08.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 27 Sep 2012 15:22:53 +0530 Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8R9qnoj25952462 for ; Thu, 27 Sep 2012 15:22:49 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8R9qmkp032688 for ; Thu, 27 Sep 2012 19:52:49 +1000 Message-ID: <506421E8.2020105@linux.vnet.ibm.com> Date: Thu, 27 Sep 2012 17:52:40 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1348712642-4427-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1348712642-4427-3-git-send-email-xiawenc@linux.vnet.ibm.com> <50640D90.7040005@redhat.com> In-Reply-To: <50640D90.7040005@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 2/5] libqblock type defines List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, eblake@redhat.com I will change the code for most of comments, but have some response below. > Il 27/09/2012 04:23, Wenchao Xia ha scritto: >> This patch contains type and macro defines used in APIs, one file for public >> usage by user, one for libqblock internal usage. >> >> Signed-off-by: Wenchao Xia >> --- >> libqblock/libqblock-internal.h | 57 +++++++++ >> libqblock/libqblock-types.h | 268 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 325 insertions(+), 0 deletions(-) >> create mode 100644 libqblock/libqblock-internal.h >> create mode 100644 libqblock/libqblock-types.h >> >> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h >> new file mode 100644 >> index 0000000..d3e983c >> --- /dev/null >> +++ b/libqblock/libqblock-internal.h >> @@ -0,0 +1,57 @@ >> +/* >> + * 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" >> + >> +#include "block.h" >> +#include "block_int.h" >> +#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)) >> + >> +#define CLEAN_FREE(p) { \ >> + FUNC_FREE(p); \ >> + (p) = NULL; \ >> +} >> + >> +/* 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 */ >> +}; >> + >> +#define G_LIBQBLOCK_ERROR g_libqbock_error_quark() >> + >> +static inline GQuark g_libqbock_error_quark(void) >> +{ >> + return g_quark_from_static_string("g-libqblock-error-quark"); >> +} >> +#endif >> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h >> new file mode 100644 >> index 0000000..948ab8a >> --- /dev/null >> +++ b/libqblock/libqblock-types.h >> @@ -0,0 +1,268 @@ >> +/* >> + * 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 >> +#else >> + #warning : gcc compiler version < 4, symbols can not be hidden. > > You don't need to warn. If LIBQB_BUILD is not defined, DLL_PUBLIC would > be empty anyway. If LIBQB_BUILD is defined, you know GCC >= 4.0 because > you pass -fvisibility=hidden from the Makefile. > >> +#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 QBlockProtType { >> + QB_PROT_NONE = 0, >> + QB_PROT_FILE, >> + QB_PROT_MAX >> +} QBlockProtType; > > Use QBlockProtocol, QB_PROTO_*. > >> +typedef struct QBlockProtOptionFile { >> + const char *filename; >> +} QBlockProtOptionFile; > > Use QBlockProtocolOptionsFile > >> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512) >> +typedef union QBlockProtOptionsUnion { >> + QBlockProtOptionFile o_file; >> + uint8_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE]; >> +} QBlockProtOptionsUnion; > > Not really options, because they are required. I would just not name > the union, and embed it in the struct. Also please change it to > > uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE / 8]; > > (Sorry for not thinking about this before). This will fix the alignment > and ensure that future changes to the union do not change the ABI. > You are right, I forgot alignment issue before. >> +/** >> + * struct QBlockProtInfo: contains information about how to find the image >> + * >> + * @prot_type: protocol type, now only support FILE. >> + * @prot_op: protocol related options. >> + */ >> +typedef struct QBlockProtInfo { >> + QBlockProtType prot_type; >> + QBlockProtOptionsUnion prot_op; >> +} QBlockProtInfo; > > What about QBlockLocation instead? Or QBlockLocationInfo. > >> + >> +/* format related options */ >> +typedef enum QBlockFmtType { >> + 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 >> +} QBlockFmtType; > > QBlockFormat; similarly remove "Type" from all enums. > >> +typedef struct QBlockFmtOptionCow { >> + uint64_t virt_size; >> + QBlockProtInfo backing_loc; >> +} QBlockFmtOptionCow; > > QBlockFormatOptionsCOW. Similarly use the plural for all other formats. > >> +typedef struct QBlockFmtOptionQed { >> + uint64_t virt_size; >> + QBlockProtInfo backing_loc; >> + QBlockFmtType backing_fmt; >> + uint64_t cluster_size; /* unit is bytes */ >> + uint64_t table_size; /* unit is clusters */ >> +} QBlockFmtOptionQed; > > Similarly use uppercase for QED, QCOW, etc. > >> +typedef struct QBlockFmtOptionQcow { >> + uint64_t virt_size; >> + QBlockProtInfo backing_loc; >> + bool encrypt; >> +} QBlockFmtOptionQcow; >> + >> +/* "Compatibility level (0.10 or 1.1)" */ >> +typedef enum QBlockFmtOptionQcow2CptLv { >> + QBO_FMT_QCOW2_CPT_NONE = 0, >> + QBO_FMT_QCOW2_CPT_V010, >> + QBO_FMT_QCOW2_CPT_V110, >> +} QBlockFmtOptionQcow2CptLv; > > Please use QBO_ instead of QB_ throughout. Also write COMPAT instead of > CPT, and remove CPT_NONE since 0.10 is the default: > __NONE is used to indicate whether this property is set or get, so it is actually have 3 status than just 2: Not set, V010, V110. It is the same reason that I use __NONE in bool values, especially __NONE could represent it is not got in information retrieving. Maybe I should rename them to __NOTSET. > typedef enum QBlockFmtOptionQCOW2Compat { > QB_FMT_QCOW2_COMPAT_V0_10, > QB_FMT_QCOW2_COMPAT_V1_1, > } QBlockFmtOptionQCOW2Compat > >> +/* off or metadata */ >> +typedef enum QBlockFmtOptionQcow2PreAllocType { >> + QBO_FMT_QCOW2_PREALLOC_NONE = 0, >> + QBO_FMT_QCOW2_PREALLOC_OFF, >> + QBO_FMT_QCOW2_PREALLOC_METADATA, >> +} QBlockFmtOptionQcow2PreAllocType; > > Please remove NONE since OFF is the default. However, I would use the enum. > >> +typedef struct QBlockFmtOptionQcow2 { >> + uint64_t virt_size; >> + QBlockProtInfo backing_loc; >> + QBlockFmtType backing_fmt; >> + bool encrypt; >> + uint64_t cluster_size; /* unit is bytes */ >> + QBlockFmtOptionQcow2CptLv cpt_lv; >> + QBlockFmtOptionQcow2PreAllocType pre_mode; >> +} QBlockFmtOptionQcow2; >> + >> +typedef struct QBlockFmtOptionRaw { >> + uint64_t virt_size; >> +} QBlockFmtOptionRaw; >> + >> +typedef struct QBlockFmtOptionRbd { >> + uint64_t virt_size; >> + uint64_t cluster_size; >> +} QBlockFmtOptionRbd; >> + >> +/* off or full */ >> +typedef enum QBlockFmtOptionSheepdogPreAllocType { >> + QBO_FMT_SD_PREALLOC_NONE = 0, >> + QBO_FMT_SD_PREALLOC_OFF, >> + QBO_FMT_SD_PREALLOC_FULL, >> +} QBlockFmtOptionSheepdogPreAllocType; > > The default is false, so just make it a bool. > >> +typedef struct QBlockFmtOptionSheepdog { >> + uint64_t virt_size; >> + QBlockProtInfo backing_loc; >> + QBlockFmtOptionSheepdogPreAllocType pre_mode; >> +} QBlockFmtOptionSheepdog; >> + >> +typedef enum QBlockFmtOptionVdiPreAllocType { >> + QBO_FMT_VDI_PREALLOC_NONE = 0, >> + QBO_FMT_VDI_PREALLOC_FALSE, >> + QBO_FMT_VDI_PREALLOC_TRUE, >> +} QBlockFmtOptionVdiPreAllocType; > > Please remove NONE, and replace FALSE/TRUE with OFF/METADATA (same as > qcow2). > >> +typedef struct QBlockFmtOptionVdi { >> + uint64_t virt_size; >> + uint64_t cluster_size; >> + QBlockFmtOptionVdiPreAllocType pre_mode; >> +} QBlockFmtOptionVdi; >> + >> +/* whether compact to vmdk verion 6 */ >> +typedef enum QBlockFmtOptionVmdkCptLv { >> + QBO_FMT_VMDK_CPT_NONE = 0, >> + QBO_FMT_VMDK_CPT_VMDKV6_FALSE, >> + QBO_FMT_VMDK_CPT_VMDKV6_TRUE, >> +} QBlockFmtOptionVmdkCptLv; > > Here the default is false, so just make it a bool. > >> +/* vmdk flat extent format, values: >> +"{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | >> +twoGbMaxExtentFlat | streamOptimized} */ >> +typedef enum QBlockFmtOptionVmdkSubfmtType { >> + QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE = 0, >> + QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE, > > You can remove QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE (whose more > appropriate name would be just ...SUBFMT_NONE) since the default is > QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE. > >> + QBO_FMT_VMDK_SUBFMT_MONOLITHIC_FLAT, >> + QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_SPARSE, >> + QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_FLAT, >> + QBO_FMT_VMDK_SUBFMT_STREAM_OPTIMIZED, >> +} QBlockFmtOptionVmdkSubfmtType; >> + >> +typedef struct QBlockFmtOptionVmdk { >> + uint64_t virt_size; >> + QBlockProtInfo backing_loc; >> + QBlockFmtOptionVmdkCptLv cpt_lv; >> + QBlockFmtOptionVmdkSubfmtType subfmt; >> +} QBlockFmtOptionVmdk; >> + >> +/* "{dynamic (default) | fixed} " */ >> +typedef enum QBlockFmtOptionVpcSubfmtType { >> + QBO_FMT_VPC_SUBFMT_NONE = 0, >> + QBO_FMT_VPC_SUBFMT_DYNAMIC, >> + QBO_FMT_VPC_SUBFMT_FIXED, >> +} QBlockFmtOptionVpcSubfmtType; > > Again, please remove NONE. > >> +typedef struct QBlockFmtOptionVpc { >> + uint64_t virt_size; >> + QBlockFmtOptionVpcSubfmtType subfmt; >> +} QBlockFmtOptionVpc; >> + >> +#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2) >> +typedef union QBlockFmtOptionsUnion { >> + QBlockFmtOptionCow o_cow; >> + QBlockFmtOptionQed o_qed; >> + QBlockFmtOptionQcow o_qcow; >> + QBlockFmtOptionQcow2 o_qcow2; >> + QBlockFmtOptionRaw o_raw; >> + QBlockFmtOptionRbd o_rbd; >> + QBlockFmtOptionSheepdog o_sheepdog; >> + QBlockFmtOptionVdi o_vdi; >> + QBlockFmtOptionVmdk o_vmdk; >> + QBlockFmtOptionVpc o_vpc; >> + uint8_t reserved[QBLOCK_FMT_OPTIONS_UNION_SIZE]; > > Similarly use uint64_t here, and just embed the union in the struct. > >> +} QBlockFmtOptionsUnion; >> +typedef struct QBlockFmtInfo { >> + QBlockFmtType fmt_type; >> + QBlockFmtOptionsUnion fmt_op; >> +} QBlockFmtInfo; > > QBlockFormatInfo. > >> +/** >> + * 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; >> + QBlockProtInfo *backing_loc; >> + bool *encrypt; >> +} QBlockStaticInfoAddr; > > Why the indirection? > It helps user to get these important members, otherwise user will need Switch (fmt) { case RAW: .... case QCOW2: ... } for every attribute. The indirection address will let user directly access the members. >> +/** >> + * QBlockStaticInfo: information about the block image. >> + * >> + * @member_addr: contains pointer which can be used to access the structure. >> + * @loc: location information. >> + * @fmt: format information. >> + * @sector_size: how many bytes in a sector, it is 512 usually. >> + */ >> +typedef struct QBlockStaticInfo { >> + QBlockStaticInfoAddr *member_addr; >> + QBlockProtInfo loc; >> + QBlockFmtInfo fmt; >> + int sector_size; >> +} QBlockStaticInfo; >> + >> + >> +#endif >> > -- Best Regards Wenchao Xia