From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41004) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwqJH-0001hE-Kk for qemu-devel@nongnu.org; Thu, 02 Aug 2012 03:59:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwqJ9-0004Ph-GE for qemu-devel@nongnu.org; Thu, 02 Aug 2012 03:59:07 -0400 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:35472) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwqJ7-0004Oq-VU for qemu-devel@nongnu.org; Thu, 02 Aug 2012 03:58:59 -0400 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 2 Aug 2012 13:28:51 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q727wiSJ19267820 for ; Thu, 2 Aug 2012 13:28:47 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q72DSIx8030374 for ; Thu, 2 Aug 2012 18:58:18 +0530 Message-ID: <501A3301.9000102@linux.vnet.ibm.com> Date: Thu, 02 Aug 2012 15:57:53 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1343812186-11594-1-git-send-email-xiawenc@linux.vnet.ibm.com> <501908AA.10400@redhat.com> In-Reply-To: <501908AA.10400@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] [RFC] libqblock draft code v1 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com 于 2012-8-1 18:44, Paolo Bonzini 写道: > Il 01/08/2012 11:09, Wenchao Xia ha scritto: >> This patch encapsulate qemu general block layer to provide block >> services. API are declared in libqblock.h. libqblock-test.c >> simulate library consumer's behaviors. Make libqblock-test could >> build the code. >> For easy this patch does not form a dynamic libarary yet. > > Thanks, it's a pretty good start! > >> >> Signed-off-by: Wenchao Xia >> --- >> Makefile | 4 +- >> libqblock-test.c | 56 +++++++++++++++ >> libqblock.c | 199 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> libqblock.h | 72 ++++++++++++++++++++ >> 4 files changed, 330 insertions(+), 1 deletions(-) >> create mode 100644 libqblock-test.c >> create mode 100644 libqblock.c >> create mode 100644 libqblock.h >> >> diff --git a/Makefile b/Makefile >> index 621cb86..6a34be6 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -152,7 +152,6 @@ vscclient$(EXESUF): $(libcacard-y) $(oslib-obj-y) $(trace-obj-y) qemu-timer-comm >> $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $^ $(libcacard_libs) $(LIBS)," LINK $@") >> >> ###################################################################### >> - >> qemu-img.o: qemu-img-cmds.h >> >> tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \ >> @@ -160,6 +159,9 @@ tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \ >> iohandler.o cutils.o iov.o async.o >> tools-obj-$(CONFIG_POSIX) += compatfd.o >> >> +libqblock-test$(EXESUF): libqblock.o libqblock-test.o $(tools-obj-y) $(block-obj-y) >> + $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $^ $(LIBS)," LINK $@") >> + >> qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) >> qemu-nbd$(EXESUF): qemu-nbd.o $(tools-obj-y) $(block-obj-y) >> qemu-io$(EXESUF): qemu-io.o cmd.o $(tools-obj-y) $(block-obj-y) >> diff --git a/libqblock-test.c b/libqblock-test.c >> new file mode 100644 >> index 0000000..663111e >> --- /dev/null >> +++ b/libqblock-test.c >> @@ -0,0 +1,56 @@ >> +#include "libqblock.h" >> + >> +#include >> +#include >> + >> + >> +unsigned char buf0[1024]; >> +unsigned char buf1[1024] = {4, 0, 0, 2}; >> +int main(int argc, char **argv) >> +{ >> + struct QBlockState i_qbs; >> + struct QBlockOpenOption i_qboo; >> + struct QBlockCreateOption i_qbco; >> + struct QBlockInfo i_qbi; >> + char *filename; >> + >> + int i; >> + unsigned long op_size = 512; >> + unsigned long op_start = 1024; >> + >> + filename = argv[1]; >> + printf("qemu test, file name is %s.\n", filename); >> + >> + libqblock_init(); >> + >> + qbs_init(&i_qbs); >> + memset(&i_qbco, 0, sizeof(struct QBlockCreateOption)); >> + >> + i_qbco.filename = filename; >> + i_qbco.fmt = (char *)"qcow2"; >> + i_qbco.flag = BDRV_O_NOCACHE; >> + i_qbco.size = 512 * 1024 * 1024; >> + qb_create(&i_qbs, &i_qbco); >> + >> + i_qboo.filename = filename; >> + i_qboo.flag = BDRV_O_RDWR; >> + qb_open(&i_qbs, &i_qboo); >> + >> + qb_write(&i_qbs, op_start, op_size, buf1); >> + qb_read(&i_qbs, op_start, op_size, buf0); >> + for (i = 0; i < op_size; i++) { >> + if (buf0[i] != buf1[i]) { >> + printf("unmatch found at %d.\n", i); >> + break; >> + } >> + } >> + qbi_init(&i_qbi); >> + qb_getinfo(&i_qbs, &i_qbi); >> + qbi_print_test(&i_qbi); >> + qbi_uninit(&i_qbi); >> + >> + qb_close(&i_qbs); >> + qb_delete(&i_qbs, filename); >> + qbs_uninit(&i_qbs); >> + return 0; >> +} >> diff --git a/libqblock.c b/libqblock.c >> new file mode 100644 >> index 0000000..00b6649 >> --- /dev/null >> +++ b/libqblock.c >> @@ -0,0 +1,199 @@ >> +#include "libqblock.h" >> + >> +#include >> + >> +#define QB_ERR_MEM_ERR (-1) >> +#define QB_ERR_INTERNAL_ERR (-2) >> +#define QB_ERR_INVALID_PARAM (-3) >> + >> +#define FUNC_FREE g_free >> + >> +#define CLEAN_FREE(p) { \ >> + FUNC_FREE(p); \ >> + (p) = NULL; \ >> +} >> + >> +/* try string dup and check if it succeed, dest would be freed before dup */ >> +#define SAFE_STRDUP(dest, src, ret, err_v) { \ >> + if ((ret) != (err_v)) { \ >> + if ((dest) != NULL) { \ >> + FUNC_FREE(dest); \ >> + } \ >> + (dest) = strdup(src); \ >> + if ((dest) == NULL) { \ >> + (ret) = (err_v); \ >> + } \ >> + } \ >> +} > > I don't think this is particularly useful. > The macro was used to take care of OOM in every strdup and report that in returns, could g_strdup do that? >> + >> +int libqblock_init(void) >> +{ >> + bdrv_init(); >> + return 0; >> +} >> + >> +int qbs_init(struct QBlockState *qbs) >> +{ >> + memset(qbs, 0, sizeof(struct QBlockState)); >> + qbs->bdrvs = bdrv_new("hda"); >> + if (qbs->bdrvs == NULL) { >> + return QB_ERR_INTERNAL_ERR; >> + } >> + return 0; >> +} >> + >> +int qbs_uninit(struct QBlockState *qbs) >> +{ >> + CLEAN_FREE(qbs->filename); >> + if (qbs->bdrvs == NULL) { >> + return QB_ERR_INVALID_PARAM; >> + } >> + bdrv_delete(qbs->bdrvs); >> + return 0; >> +} >> + >> +int qb_create(struct QBlockState *qbs, struct QBlockCreateOption *op) >> +{ >> + int ret; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + >> + ret = bdrv_img_create(op->filename, op->fmt, op->base_filename, >> + op->base_fmt, op->options, op->size, op->flag); >> + return 0; >> +} >> + >> +int qb_open(struct QBlockState *qbs, struct QBlockOpenOption *op) >> +{ >> + int ret; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + ret = bdrv_open(bs, op->filename, op->flag, NULL); >> + if (ret < 0) { >> + return ret; >> + } >> + >> + SAFE_STRDUP(qbs->filename, op->filename, ret, QB_ERR_MEM_ERR); >> + return ret; >> +} >> + >> +int qb_close(struct QBlockState *qbs) >> +{ >> + int ret = 0; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + bdrv_close(bs); >> + return ret; >> +} >> + >> +int qb_delete(struct QBlockState *qbs, const char *filename) >> +{ >> + return unlink(filename); >> +} >> + >> +/* ignore case that len is not alligned to 512 now. */ >> +int qb_read(struct QBlockState *qbs, unsigned long start, unsigned long len, >> + unsigned char *buf) >> +{ >> + int ret; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + >> + ret = bdrv_read(bs, start / 512, >> + buf, len / 512); >> + return ret; >> +} >> + >> +/* ignore case that len is not alligned to 512 now. */ >> +int qb_write(struct QBlockState *qbs, unsigned long start, unsigned long len, >> + unsigned char *buf) >> +{ >> + int ret; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + ret = bdrv_write(bs, start / 512, >> + buf, len / 512); >> + return ret; >> +} >> + >> +int qb_flush(struct QBlockState *qbs) >> +{ >> + int ret = 0; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + ret = bdrv_flush(bs); >> + return ret; >> +} >> + >> +int qbi_init(struct QBlockInfo *info) > > Should return void. > yes, it is useless now, but maybe in future it may fail. Returning int for every API results in only one more "eax store", I hope I can keep all API returns indicate success/fail, while add comments to say which API would not fail now. >> +{ >> + memset(info, 0, sizeof(struct QBlockInfo)); >> + return 0; >> +} >> + >> +int qbi_uninit(struct QBlockInfo *info) > > Should return void. > >> +{ >> + CLEAN_FREE(info->filename); >> + CLEAN_FREE(info->fmt); >> + CLEAN_FREE(info->backing_filename); >> + CLEAN_FREE(info->sn_tab); >> + return 0; >> +} >> + >> +int qbi_print_test(struct QBlockInfo *info) >> +{ >> + printf("name:%s, fmt %s, virt_size %ld, allocated_size %ld, encryt %d, " >> + "back_file %s, sn_tab %p, sn_num %d, cluster size %d, " >> + "vm_state_offset %ld, dirty %d.\n", >> + info->filename, info->fmt, info->virt_size, info->allocated_size, >> + info->encrypt_flag, >> + info->backing_filename, info->sn_tab, info->sn_tab_num, >> + info->cluster_size, >> + info->vm_state_offset, info->is_dirty); >> + return 0; >> +} > > This function is not needed in the library. > ok. >> +int qb_getinfo(struct QBlockState *qbs, struct QBlockInfo *info) >> +{ >> + int ret = 0; >> + BlockDriverState *bs; >> + const char *tmp; >> + BlockDriverInfo bdi; >> + uint64_t total_sectors; >> + char backing_filename[1024]; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + SAFE_STRDUP(info->filename, qbs->filename, ret, QB_ERR_MEM_ERR); >> + tmp = bdrv_get_format_name(bs); >> + SAFE_STRDUP(info->fmt, tmp, ret, QB_ERR_MEM_ERR); >> + bdrv_get_geometry(bs, &total_sectors); >> + info->virt_size = total_sectors * 512; >> + info->allocated_size = bdrv_get_allocated_file_size(bs); >> + info->encrypt_flag = bdrv_is_encrypted(bs); >> + bdrv_get_full_backing_filename(bs, backing_filename, >> + sizeof(backing_filename)); >> + if (backing_filename[0] != '\0') { >> + SAFE_STRDUP(info->backing_filename, backing_filename, ret, >> + QB_ERR_MEM_ERR); > > Here you need to clear info->backing_filename if there is no backing > file. But see below for an alternate proposal. > >> + } >> + info->sn_tab_num = bdrv_snapshot_list(bs, &(info->sn_tab)); >> + if (info->sn_tab_num < 0) { >> + info->sn_tab_num = 0; >> + } >> + if (bdrv_get_info(bs, &bdi) >= 0) { >> + info->cluster_size = bdi.cluster_size; >> + info->vm_state_offset = bdi.vm_state_offset; >> + info->is_dirty = bdi.is_dirty; >> + } else { >> + info->cluster_size = -1; >> + info->vm_state_offset = -1; >> + info->is_dirty = -1; >> + } >> + return ret; >> +} >> diff --git a/libqblock.h b/libqblock.h >> new file mode 100644 >> index 0000000..64a8b96 >> --- /dev/null >> +++ b/libqblock.h >> @@ -0,0 +1,72 @@ >> +#ifndef LIBQBLOCK_H >> +#define LIBQBLOCK_H >> + >> +#include "block.h" > > Please do not include this from libqblock.h. You can use BUILD_BUG_ON > to ensure that any constant you define here matches the value in block.h. > OK, let me check how to use BUILD_BUG_ON. >> +/* details should be hidden to user */ >> +struct QBlockState { >> + void *bdrvs; >> + char *filename; >> +}; >> + >> +/* libarary init or uninit */ >> +int libqblock_init(void); >> + >> +/* qbs init and uninit */ >> +int qbs_init(struct QBlockState *qbs); >> +int qbs_uninit(struct QBlockState *qbs); >> + >> +/* file open and close */ >> +struct QBlockOpenOption { > > Please add a struct_size member that the caller should initialize to > sizeof(QBlockOpenOption), and check it in qb_open. This will let us add > more fields in the future (with some care). > Seems a good way to make it compatiable, but I am worried how to make the checking logical simple for every structure. for eg: if (sizeof(QBlockOpenOption) != qpoo->struct_size) { if (qpoo->struct_size == 32) { /* Verion 1 */ } elseif (qpoo->struct_size == 40) { /* Verion 2 */ } ..... } This seems complicated. Instead we can keep some space in the structureas: struct QBlockOpenOption { const char *filename; const char *fmt; uint32 reserved[32] }; And make sure the structure size do not change at minor version change, then in library init: int libqblock_init(int verson) to check if user is using a compatiable version's header files. > Also, we shouldn't encode the protocol in the filename. This is not an > easily extensible interface. If for now we want to only support files, > we should add an > > enum QBlockProtocol { > QB_PROTO_FILE, > QB_PROTO_MAX = QB_PROTO_FILE > } > > fail qb_open if the value is out of range. > > To implement this, for now you can just fail qb_open if > path_has_protocol(filename) returns true. In the future we can refine > this to allow opening a file with a colon in its name. > >> + char *filename; > > const char * > > We also want to pass the format, so add another "const char *format". > > OK. >> + int flag; >> +}; >> + >> +struct QBlockCreateOption { > > Please add the struct_size member here. > >> + char *filename; >> + char *fmt; >> + char *base_filename; >> + char *base_fmt; >> + char *options; > > Add const in all these places. > right. >> + unsigned long size; >> + int flag; >> +}; >> + >> +int qb_open(struct QBlockState *qbs, struct QBlockOpenOption *op); >> +int qb_close(struct QBlockState *qbs); >> + >> +int qb_create(struct QBlockState *qbs, struct QBlockCreateOption *op); >> +int qb_delete(struct QBlockState *qbs, const char *filename); > > Is this needed? need a way to create new image, merge this function into open makes the option in qb_open a little strange, many option not used in normal open should be present in it, so I splitted the API out. > > We also need this: > > /* format access */ > bool qb_supports_format(const char *format); > bool qb_supports_protocol(enum QBlockProtocol); > will added them in next version. >> + >> +/* information */ >> +struct QBlockInfo { > > You could add a sizeof() member here too. Initialize it in qbi_init and > check it in qbi_uninit/qb_getinfo. > > But I'm not sure if it's a good idea to handle allocation like this for > QBlockInfo. If you make qb_getinfo allocate its own struct QBlockInfo, > you can get rid of qbi_init and SAFE_STRDUP. > that will forces an allocation on heap, user need to: info = qb_getinfo(); qbi_free(info); all parameters are freed and info itself is freed, but info still have invalid pointer value,so qbi_init/qbi_uninit pairs seems better. >> + /* basic info */ >> + char *filename; >> + char *fmt; >> + /* advance info */ >> + unsigned long virt_size; >> + unsigned long allocated_size; >> + int encrypt_flag; >> + char *backing_filename; >> + QEMUSnapshotInfo *sn_tab; > > QEMUSnapshotInfo should not be visible to clients of the library. You > need a separate QBlockSnapshotList type. > right. >> + int sn_tab_num; >> + >> + /* in bytes, 0 if irrelevant */ >> + int cluster_size; >> + int64_t vm_state_offset; >> + int is_dirty; >> +}; >> + >> +/* sync access */ >> +int qb_read(struct QBlockState *qbs, unsigned long start, unsigned long len, >> + unsigned char *buf); >> +int qb_write(struct QBlockState *qbs, unsigned long start, unsigned long len, >> + unsigned char *buf); >> +int qb_flush(struct QBlockState *qbs); >> + >> +/* get info */ >> +int qbi_init(struct QBlockInfo *info); >> +int qbi_uninit(struct QBlockInfo *info); > > Following the suggestion above, this would become qb_free_info and would > also free the parameter. > >> +int qbi_print_test(struct QBlockInfo *info); >> +int qb_getinfo(struct QBlockState *qbs, struct QBlockInfo *info); > > Please call this qb_get_info. > OK. >> +#endif >> > > > Paolo > -- Best Regards Wenchao Xia