On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiaw...@linux.vnet.ibm.com> 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 <xiaw...@linux.vnet.ibm.com> > --- > 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. > 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. > @@ -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 <stdarg.h> > +#include <stdio.h> > + > + > +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? :) > + 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*? > + 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. > + 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. > + > + 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 <unistd.h> > + > +#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. > +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. > + } > + bdrv_delete(qbs->bdrvs); > + return 0; > +} > + > +int qb_create(struct QBlockState *qbs, struct QBlockCreateOption *op) Why are some functions called qbs_*() and others qb_*()? > +{ > + 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. > + > + 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. > + } > + > + 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. > +} > + > +/* 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. > +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. Stefan