From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42898) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0si0-0004Zs-NZ for qemu-devel@nongnu.org; Mon, 13 Aug 2012 07:21:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T0shw-00034H-GY for qemu-devel@nongnu.org; Mon, 13 Aug 2012 07:21:20 -0400 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:52201) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0shv-00033n-FL for qemu-devel@nongnu.org; Mon, 13 Aug 2012 07:21:16 -0400 Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 13 Aug 2012 21:20:13 +1000 Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7DBCLTT19136598 for ; Mon, 13 Aug 2012 21:12:23 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7DBL1Il020516 for ; Mon, 13 Aug 2012 21:21:01 +1000 Message-ID: <5028E313.40702@linux.vnet.ibm.com> Date: Mon, 13 Aug 2012 19:20:51 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1344507137-11173-1-git-send-email-xiawenc@linux.vnet.ibm.com> <5024EA37.3020004@redhat.com> In-Reply-To: <5024EA37.3020004@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [qemu-devel] [PATCH V2 2/3] [RFC] libqblock-API design List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: stefanha@gmail.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, pbonzini@redhat.com 于 2012-8-10 19:02, Kevin Wolf 写道: > Am 09.08.2012 12:12, schrieb Wenchao Xia: >> This patch is the API design. >> >> Signed-off-by: Wenchao Xia >> --- >> libqblock.c | 670 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> libqblock.h | 447 +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 1117 insertions(+), 0 deletions(-) >> create mode 100644 libqblock.c >> create mode 100644 libqblock.h > > Ignoring the implementation for now as the design should be visible in > the header file. > >> diff --git a/libqblock.h b/libqblock.h >> new file mode 100644 >> index 0000000..d2e9502 >> --- /dev/null >> +++ b/libqblock.h >> @@ -0,0 +1,447 @@ >> +/* >> + * Copyright IBM Corp. 2012 >> + * >> + * Authors: >> + * Wenchao Xia >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA >> + */ >> + >> +#ifndef LIBQBLOCK_H >> +#define LIBQBLOCK_H >> + >> +#include >> +#include >> +#include >> + >> +#define bool _Bool > > Why not use stdbool.h? > forgot to use that, will change it, thanks. >> + >> +#define QB_ERR_MEM_ERR (-1) >> +#define QB_ERR_INTERNAL_ERR (-2) >> +#define QB_ERR_INVALID_PARAM (-3) > > qemu uses errno values internally, I think they would make sense in the > library interface as well. > >> + >> +/* this library is designed around this core struct. */ >> +struct QBlockState; >> + >> +/* >> + libarary init >> + This function get the library ready to use. >> + */ >> +void libqblock_init(void); >> + >> +/* >> + create a new qbs object >> + params: >> + qbs: out, pointer that will receive created obj. >> + return: >> + 0 on succeed, negative on failure. >> + */ >> +int qb_state_new(struct QBlockState **qbs); >> + >> +/* >> + delete a qbs object >> + params: >> + qbs: in, pointer that will be freed. *qbs will be set to NULL. >> + return: >> + void. >> + */ >> +void qb_state_free(struct QBlockState **qbs); > > What happens if the qbs is open? Will it be flushed and closed? If so, > can it fail and we need to allow an error return? > user should call qb_close() if he have called qb_open(), other wise unexpected thing happens such as this file is not flushed. I need document this in the comments. >> + >> +/* flag used in open and create */ >> +#define LIBQBLOCK_O_RDWR 0x0002 >> +/* open the file read only and save writes in a snapshot */ >> +#define LIBQBLOCK_O_SNAPSHOT 0x0008 >> +/* do not use the host page cache */ >> +#define LIBQBLOCK_O_NOCACHE 0x0020 >> +/* use write-back caching */ >> +#define LIBQBLOCK_O_CACHE_WB 0x0040 >> +/* use native AIO instead of the thread pool */ >> +#define LIBQBLOCK_O_NATIVE_AIO 0x0080 >> +/* don't open the backing file */ >> +#define LIBQBLOCK_O_NO_BACKING 0x0100 >> +/* disable flushing on this disk */ >> +#define LIBQBLOCK_O_NO_FLUSH 0x0200 >> +/* copy read backing sectors into image */ >> +#define LIBQBLOCK_O_COPY_ON_READ 0x0400 >> +/* consistency hint for incoming migration */ >> +#define LIBQBLOCK_O_INCOMING 0x0800 >> + >> +#define LIBQBLOCK_O_CACHE_MASK \ >> + (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH) >> + >> +enum QBlockProtocol { >> + QB_PROTO_FILE = 0, >> + QB_PROTO_MAX >> +}; >> + >> +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 >> +}; > > Not sure if this is a good idea with respect to extensibility. Today you > only need to create a new block/foo.c and add it to the Makefile in > order to add a new format and protocol. It would be better if the > library could make use of it without changing these enums, e.g. by > referring to formats by string (possibly getting a struct referring to a > format by something like qblk_get_format("raw")) > qblk_get_format("raw") seems a good way solve the option difference, maybe user call it as: void *op = qblk_get_format_option("raw"); struct QBlockOption_raw *o_raw = (struct QBlockOption_raw *)op; o_raw.virt_size = 16 * 1024; I think this is good, with the cost of user have to deal string instead of simpler enum integer. >> + >> +/* block target location info, it include all information about how to find >> + the image */ >> +struct QBlockLocInfo { >> + int struct_size; >> + const char *filename; >> + enum QBlockProtocol protocol; >> +}; > > This is a relatively simplistic view of the world. It works okay for > local image files (which is probably the most common use case for the > library), but you get into trouble as soon as you want to build some > less trivial structures, like something including blkdebug. > > Eventually, this leads straight into -blockdev discussions, so maybe we > should indeed go with a very simplistic approach for the start, but > always be aware that a more general solution is to be expected and > should be possible to fit in naturally. > if blkdebug is used for debug block I/O, maybe we can sort it into another protocol. >> + >> +/* how to open the image */ >> +struct QBlockOptionOpen { >> + int struct_size; >> + struct QBlockLocInfo o_loc; /* how to find */ >> + enum QBlockFormat o_fmt_type; /* how to extract */ >> + int o_flag; /* how to control */ >> +}; >> + >> +/* >> + create a new QBlockOptionOpen structure. >> + params: >> + op: out, pointer that will receive created structure. >> + return: >> + 0 on succeed, negative on failure. >> + */ >> +static inline int qb_oo_new(struct QBlockOptionOpen **op) >> +{ >> + *op = calloc(1, sizeof(struct QBlockOptionOpen)); >> + if (*op == NULL) { >> + return QB_ERR_MEM_ERR; >> + } >> + (*op)->struct_size = sizeof(struct QBlockOptionOpen); >> + (*op)->o_loc.struct_size = sizeof(struct QBlockLocInfo); >> + return 0; >> +} >> + >> +/* >> + free QBlockOptionOpen structure. >> + params: >> + op: in, *op will be set as NULL after called. >> + return: >> + void >> + */ >> +static inline void qb_oo_free(struct QBlockOptionOpen **op) >> +{ >> + free(*op); >> + *op = NULL; >> +} >> + >> +/* >> + open a block object. >> + params: >> + qbs: in, pointer to QBlockState. >> + op: in, options for open. >> + return: >> + 0 on success, negative on fail. >> + */ >> +int qb_open(struct QBlockState *qbs, struct QBlockOptionOpen *op); > > No further explanation on what negative values there are allowed? In > practice I guess -errno, but will it be officially undefined? > > Will there be a function that allows getting an error message if it has > failed? > Yes, there is a API doint that. >> + >> +/* >> + close a block object. >> + params: >> + qbs: in, pointer to QBlockState. >> + return: >> + void. >> + */ >> +void qb_close(struct QBlockState *qbs); > > Does it imply a flush? Do we need an error return then? > No flush now, will flush and document it next time. >> + >> +/* format related options, struct_size must be set. */ >> +struct QBlockOption_cow { >> + int struct_size; >> + size_t virt_size; >> + const char *backing_file; >> + int backing_flag; >> +}; >> + >> +struct QBlockOption_qed { >> + int struct_size; >> + size_t virt_size; >> + const char *backing_file; >> + enum QBlockFormat backing_fmt; >> + int backing_flag; >> + size_t cluster_size; /* unit is bytes */ >> + size_t table_size; /* unit is clusters */ >> +}; >> + >> +struct QBlockOption_qcow { >> + int struct_size; >> + size_t virt_size; >> + const char *backing_file; >> + int backing_flag; >> + bool encrypt; >> +}; >> + >> +struct QBlockOption_qcow2 { >> + int struct_size; >> + size_t virt_size; >> + const char *compat_level; /* "Compatibility level (0.10 or 1.1)" */ >> + const char *backing_file; >> + enum QBlockFormat backing_fmt; >> + int backing_flag; >> + bool encrypt; >> + size_t cluster_size; /* unit is bytes */ >> + const char *prealloc_mode; /* off or metadata */ >> +}; >> + >> +struct QBlockOption_raw { >> + int struct_size; >> + size_t virt_size; >> +}; >> + >> +struct QBlockOption_rbd { >> + int struct_size; >> + size_t virt_size; >> + size_t cluster_size; >> +}; >> + >> +struct QBlockOption_sheepdog { >> + int struct_size; >> + size_t virt_size; >> + const char *backing_file; >> + int backing_flag; >> + const char *prealloc_mode; /* off or full */ >> +}; >> + >> +struct QBlockOption_vdi { >> + int struct_size; >> + size_t virt_size; >> + size_t cluster_size; >> + bool prealloc_mode; >> +}; >> + >> +struct QBlockOption_vmdk { >> + int struct_size; >> + size_t virt_size; >> + const char *backing_file; >> + int backing_flag; >> + bool compat_version6; >> + const char *subfmt; >> + /* vmdk flat extent format, values: >> + "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse | >> + twoGbMaxExtentFlat | streamOptimized} */ >> +}; >> + >> +struct QBlockOption_vpc { >> + int struct_size; >> + size_t virt_size; >> + const char *subfmt; /* "{dynamic (default) | fixed} " */ >> +}; > > Ouch. :-) > > But maybe it's the best we can do... > This structure brought format specified options to the surface, user could understand them easier and simply give values to the members, so this is a friendly API. But with a union implement became hard, I think the way your suggested as qblk_format_option_get("raw") could make thing easier. >> + >> +union QBlockOption_fmt { >> + struct QBlockOption_cow o_cow; >> + struct QBlockOption_qed o_qed; >> + struct QBlockOption_qcow o_qcow; >> + struct QBlockOption_qcow2 o_qcow2; >> + struct QBlockOption_raw o_raw; >> + struct QBlockOption_rbd o_rbd; >> + struct QBlockOption_sheepdog o_sheepdog; >> + struct QBlockOption_vdi o_vdi; >> + struct QBlockOption_vmdk o_vmdk; >> + struct QBlockOption_vpc o_vpc; >> +}; >> + >> +struct QBlockOptionFormat { >> + int struct_size; >> + enum QBlockFormat fmt_type; >> + union QBlockOption_fmt fmt_op; >> +}; >> + >> +/* struct_size in o_loc and o_fmt must set. To make this structure extensible, >> + all new member must be added in the tail of each structure. */ >> +struct QBlockOptionCreate { >> + int struct_size; > > size_t? > struct_size will not be big, should I also use size_t? >> + struct QBlockLocInfo o_loc; >> + struct QBlockOptionFormat o_fmt; >> +}; >> + >> +/* >> + create a new QBlockOptionCreate structure. >> + params: >> + op: out, pointer that will receive created structure. >> + fmt: format want to use. >> + return: >> + 0 on succeed, negative on failure. >> + */ >> +static inline int qb_oc_new(struct QBlockOptionCreate **op, >> + enum QBlockFormat fmt) >> +{ >> + *op = calloc(1, sizeof(struct QBlockOptionCreate)); >> + if (*op == NULL) { >> + return QB_ERR_MEM_ERR; >> + } >> + (*op)->struct_size = sizeof(struct QBlockOptionCreate); >> + (*op)->o_loc.struct_size = sizeof(struct QBlockLocInfo); >> + (*op)->o_fmt.struct_size = sizeof(struct QBlockOptionFormat); >> + >> + switch (fmt) { >> + case QB_FMT_COW: >> + (*op)->o_fmt.fmt_op.o_cow.struct_size = >> + sizeof(struct QBlockOption_cow); >> + break; >> + case QB_FMT_QED: >> + (*op)->o_fmt.fmt_op.o_qed.struct_size = >> + sizeof(struct QBlockOption_qed); >> + break; >> + case QB_FMT_QCOW: >> + (*op)->o_fmt.fmt_op.o_qcow.struct_size = >> + sizeof(struct QBlockOption_qcow); >> + break; >> + case QB_FMT_QCOW2: >> + (*op)->o_fmt.fmt_op.o_qcow2.struct_size = >> + sizeof(struct QBlockOption_qcow2); >> + break; >> + case QB_FMT_RAW: >> + (*op)->o_fmt.fmt_op.o_raw.struct_size = >> + sizeof(struct QBlockOption_raw); >> + break; >> + case QB_FMT_RBD: >> + (*op)->o_fmt.fmt_op.o_rbd.struct_size = >> + sizeof(struct QBlockOption_rbd); >> + break; >> + case QB_FMT_SHEEPDOG: >> + (*op)->o_fmt.fmt_op.o_sheepdog.struct_size = >> + sizeof(struct QBlockOption_sheepdog); >> + break; >> + case QB_FMT_VDI: >> + (*op)->o_fmt.fmt_op.o_vdi.struct_size = >> + sizeof(struct QBlockOption_vdi); >> + break; >> + case QB_FMT_VMDK: >> + (*op)->o_fmt.fmt_op.o_vmdk.struct_size = >> + sizeof(struct QBlockOption_vmdk); >> + break; >> + case QB_FMT_VPC: >> + (*op)->o_fmt.fmt_op.o_vpc.struct_size = >> + sizeof(struct QBlockOption_vpc); >> + break; >> + default: >> + break; >> + } >> + (*op)->o_fmt.fmt_type = fmt; >> + return 0; >> +} >> + This is what make implement hard, with format union, in order to form a friendly API. >> +/* >> + free QBlockOptionCreate structure. >> + params: >> + op: in, *op will be set as NULL after called. >> + return: >> + void >> + */ >> +static inline void qb_oc_free(struct QBlockOptionCreate **op) >> +{ >> + free(*op); >> + *op = NULL; >> +} >> +/* >> + create a block file. >> + params: >> + qbs: in, pointer to QBlockState. >> + op: in, create option. >> + return: >> + negative on fail, 0 on success. >> + */ >> +int qb_create(struct QBlockState *qbs, struct QBlockOptionCreate *op); >> + >> + >> +/* sync access */ >> +/* >> + block sync read. >> + params: >> + qbs: in, pointer to QBlockState. >> + buf: out, buffer that receive the content. >> + len: in, length to read. >> + offset: in, offset in the block data. >> + return: >> + negative on fail, 0 on success. >> + */ >> +int qb_read(struct QBlockState *qbs, const void *buf, size_t len, >> + off_t offset); >> + >> +/* >> + block sync read. >> + params: >> + qbs: in, pointer to QBlockState. >> + buf: in, buffer that would be wrote. >> + len: in, length to write. >> + offset: in, offset in the block data. >> + return: >> + negative on fail, 0 on success. >> + */ >> +int qb_write(struct QBlockState *qbs, const void *buf, size_t len, >> + off_t offset); >> + >> +/* >> + block sync flush. >> + params: >> + qbs: in, pointer to QBlockState. >> + return: >> + negative on fail, 0 on success. >> + */ >> +int qb_flush(struct QBlockState *qbs); >> + >> +/* information */ >> +/* image related info, static information, from user perspective. */ >> +/* now it is a plain structure, wonder if it could be foldered into embbed one >> + to reflect that format related information better. */ >> +struct QBlockInfoImage { >> + int struct_size; >> + char *filename; >> + enum QBlockProtocol protocol; >> + enum QBlockFormat format; >> + size_t virt_size; >> + /* advance info */ >> + size_t allocated_size; >> + bool encrypt; >> + char *backing_filename; >> +}; > > What about things like cluster size, dirty state etc. that only some > image formats have? > hard to resolve, enum plus sub format structure seems reasonable. My plan is using embbed structure: struct QBlockInfoImage { int struct_size; struct QBlockInfoLoc *i_loc; struct QBlockInfoFormat *i_fmt; }; with format string user got, user do a cast to specified format info. >> + >> +/* image info */ >> +/* >> + get image info. >> + params: >> + qbs: in, pointer to QBlockState. >> + info, out, pointer that would receive the information. >> + return: >> + negative on fail, 0 on success. >> + */ >> +int qb_infoimage_get(struct QBlockState *qbs, struct QBlockInfoImage **info); >> + >> +/* >> + free image info. >> + params: >> + info, in, pointer, *info would be set to NULL after function called. >> + return: >> + void. >> + */ >> +void qb_infoimage_free(struct QBlockInfoImage **info); >> + >> +/* misc */ >> +bool qb_supports_format(enum QBlockFormat fmt); >> +bool qb_supports_protocol(enum QBlockProtocol proto); >> + >> +const char *qb_error_get_detail(struct QBlockState *qbs, int *err_num); > > This needs a more detailed description. I also think that you'll want to > add a const char** parameter for human readable error messages (as > printed with qerror_report or error_set in qemu). > This API try to return human readable strings. Will add document. > Kevin > -- Best Regards Wenchao Xia