From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54354) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swqd3-0005Z3-Mf for qemu-devel@nongnu.org; Thu, 02 Aug 2012 04:19:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Swqcx-0001Ru-1F for qemu-devel@nongnu.org; Thu, 02 Aug 2012 04:19:33 -0400 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:54970) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swqcw-0001Rm-3H for qemu-devel@nongnu.org; Thu, 02 Aug 2012 04:19:26 -0400 Received: from /spool/local by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 2 Aug 2012 13:49:18 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay05.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q728JCDV22347946 for ; Thu, 2 Aug 2012 13:49:14 +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 q72DmjeM017790 for ; Thu, 2 Aug 2012 19:18:45 +0530 Message-ID: <501A37EF.7020305@linux.vnet.ibm.com> Date: Thu, 02 Aug 2012 16:18:55 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1343812186-11594-1-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: 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: Stefan Hajnoczi Cc: pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com 于 2012-8-1 20:49, Stefan Hajnoczi 写道: > On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia wrote: >> 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. >> >> 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 > > I haven't looked at Paolo's feedback yet, so please ignore any > duplicate comments :). > > Please include API documentation. I suggest doc comments in the > libqblock.h header file. > Certainly comments would be added. >> 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 \ > > Whitespace change. Please drop this. > My mistake. >> @@ -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" > > Please include GPLv2+ license headers in new source files you create. > See existing code like include/qemu/object.h for the license header > text. > >> + >> +#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; > > What does i_ mean? :) > Some kind of naming rules on Windows.:| i_ means instance, p_ means pointer, g_ means gloable, I found the naming helps me in coding, but this style would not goto the library, due to most people for Linux seems dislike it. >> + 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"; > > Why does fmt need to be char* and not const char*? > should be const, sorry. >> + i_qbco.flag = BDRV_O_NOCACHE; > > BDRV_O_* constants are not an ABI. That means they are not stable and > could change (because they are QEMU internals). > > Either BDRV_O_* needs to become a public ABI or we need public > constants for libqblock. > right, then I'll hide the options. >> + 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); > > Error handling is worth showing too. > OK, will implement it later. >> + >> + 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'm not a fan of these macros. Use g_strdup() and don't hide simple > operations like freeing and clearing a pointer. > Main purpose of it is to set ret to err_v when memory is not enough, I am not sure how to make this happens for every strdup. >> +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; > > Being able to safely close/uninitialize/etc an object multiple times > is a useful property. I would return 0 here instead of an error. > OK. >> + } >> + bdrv_delete(qbs->bdrvs); >> + return 0; >> +} >> + >> +int qb_create(struct QBlockState *qbs, struct QBlockCreateOption *op) > > Why are some functions called qbs_*() and others qb_*()? qbs_ is for the struct QBlockState itself, qb_ is for real block operation, I have not got better naming yet. > >> +{ >> + int ret; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; > > In C casting between void* and another pointer type is implicit and > there is no compiler warning. Please drop the casts. > > I think the easiest way to avoid this type cast issue is to forward > declare "typedef struct BlockDriverState BlockDriverState" in > libqblock.h. The caller will still be unable to access it directly. > OK. >> + >> + 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; > > Here you are passing QEMU internal -errno back to the user. As long > as qb_open() is documented as returning -errno this is okay, but I was > a little surprised because you declared your own error constants for > libqblock. > This is something hard to resolve for me. Because the library encapsulate general block layer so the bdrv_open() 's return is a internal state, and reports should be QB_ERR_INTERNAL_ERR, but this seems to be too little information. Unless we merge this layer to general qemu block layer and declares errors together, I don't know how to get more error info to user. >> + } >> + >> + 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); > > This is a little tricky. Some block drivers have no actual file on > disk. Others use multiple files for a single disk image. > > I suggest dropping this API for now. > Right. >> +} >> + >> +/* 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) > > unsigned long is the wrong type. It needs to be uint64_t to guarantee 64-bit. > > I also suggest using void *buf and size_t len. This is what the C > standard library APIs use for buffers and sizes. > OK. >> +int qbi_init(struct QBlockInfo *info) >> +{ >> + memset(info, 0, sizeof(struct QBlockInfo)); >> + return 0; >> +} > > If you think an API will never return an error (even in the future), > then make it return void. Otherwise all callers have to check the > return value. > >> + >> +int qbi_uninit(struct QBlockInfo *info) >> +{ >> + 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", > > Disk offsets and size need to be uint64_t with %PRIu64 format string specifiers. > OK. > Stefan > -- Best Regards Wenchao Xia