From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swqq7-0003n7-3J for qemu-devel@nongnu.org; Thu, 02 Aug 2012 04:33:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Swqq0-0005Kw-7f for qemu-devel@nongnu.org; Thu, 02 Aug 2012 04:33:03 -0400 Received: from e28smtp04.in.ibm.com ([122.248.162.4]:40513) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swqpy-0005KO-W2 for qemu-devel@nongnu.org; Thu, 02 Aug 2012 04:32:56 -0400 Received: from /spool/local by e28smtp04.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 2 Aug 2012 14:02:49 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay02.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q728WkK027525334 for ; Thu, 2 Aug 2012 14:02:46 +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 q72E2K8L005980 for ; Thu, 2 Aug 2012 19:32:20 +0530 Message-ID: <501A3B24.6000203@linux.vnet.ibm.com> Date: Thu, 02 Aug 2012 16:32:36 +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: Blue Swirl Cc: pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com 于 2012-8-2 2:04, Blue Swirl 写道: > On Wed, Aug 1, 2012 at 9: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 >> >> 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}; > > 'static' for the above, 'const' for buf1. I'd use uint8_t instead of > 'unsigned char'. > Native C type was used to simplify test codings, will correct them to platform across types. >> +int main(int argc, char **argv) >> +{ >> + struct QBlockState i_qbs; >> + struct QBlockOpenOption i_qboo; >> + struct QBlockCreateOption i_qbco; >> + struct QBlockInfo i_qbi; > > The variable names are cryptic. > >> + char *filename; >> + >> + int i; >> + unsigned long op_size = 512; >> + unsigned long op_start = 1024; > > size_t > >> + >> + 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"; > > Add 'const' to fmt field and remove cast. > >> + 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); > > mismatch > >> + 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 > > Useless. > >> + >> +#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); \ >> + } \ >> + } \ >> +} > > Just use g_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; >> + } >> + 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) > > const struct QBlockOpenOption *op > right. >> +{ >> + 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. */ > > aligned > will change. >> +int qb_read(struct QBlockState *qbs, unsigned long start, unsigned long len, >> + unsigned char *buf) > > I'd make this closer to pread() interface: > ssize_t qb_read(struct QBlockState *qbs, void *buf, size_t len, off_t offset) > OK. >> +{ >> + int ret; >> + BlockDriverState *bs; >> + >> + bs = (BlockDriverState *)qbs->bdrvs; >> + >> + ret = bdrv_read(bs, start / 512, >> + buf, len / 512); > > IIRC there is a constant for 512 somewhere. > will include that header. >> + return ret; >> +} >> + >> +/* ignore case that len is not alligned to 512 now. */ > > aligned > >> +int qb_write(struct QBlockState *qbs, unsigned long start, unsigned long len, >> + unsigned char *buf) > > Also here match pwrite: > ssize_t qb_write(struct QBlockState *qbs, const void *buf, size_t len, > off_t offset) > >> +{ >> + 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) >> +{ >> + memset(info, 0, sizeof(struct QBlockInfo)); >> + return 0; >> +} >> + >> +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", >> + 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 does not belong to the library. > >> + >> +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); >> + } >> + 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" >> + >> +/* 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 { >> + char *filename; >> + int flag; > > Possible flags should be listed as enums. > right. >> +}; >> + >> +struct QBlockCreateOption { >> + char *filename; >> + char *fmt; >> + char *base_filename; >> + char *base_fmt; >> + char *options; > > What would these be? > options is used in qemu general block layer such as -c(compress), will try alternate it to enums. >> + unsigned long size; > > uint64_t > >> + int flag; > > Possible flags should be listed as enums. > >> +}; >> + >> +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); >> + >> +/* information */ >> +struct QBlockInfo { >> + /* basic info */ >> + char *filename; >> + char *fmt; >> + /* advance info */ >> + unsigned long virt_size; >> + unsigned long allocated_size; > > uint64_t for both above. > >> + int encrypt_flag; > > bool is_encrypted; > Compared to bool, int may indicate some property is not available by (-1). So I changed bool to int. >> + char *backing_filename; >> + QEMUSnapshotInfo *sn_tab; > > Nack, don't export QEMU types directly. > >> + int sn_tab_num; > > I'm not sure this is needed. > >> + >> + /* in bytes, 0 if irrelevant */ >> + int cluster_size; >> + int64_t vm_state_offset; > > Nack, not part of block interface. > OK. Will remove this part, and collect it in another API more closed to emulator usage. >> + int is_dirty; > > bool > >> +}; >> + >> +/* 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 */ > > Comment is misplaced. > Will corrrect it. >> +int qbi_init(struct QBlockInfo *info); >> +int qbi_uninit(struct QBlockInfo *info); >> +int qbi_print_test(struct QBlockInfo *info); >> +int qb_getinfo(struct QBlockState *qbs, struct QBlockInfo *info); >> +#endif >> -- >> 1.7.1 >> >> >> > -- Best Regards Wenchao Xia