| Message ID | 1343812186-11594-1-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <xiawenc@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 > > 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 <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; > + 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 <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 don't think this is particularly useful. > + > +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. > +{ > + 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. > +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. > +/* 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). 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". > + 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. > + 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? We also need this: /* format access */ bool qb_supports_format(const char *format); bool qb_supports_protocol(enum QBlockProtocol); > + > +/* 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. > + /* 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. > + 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. > +#endif > Paolo
On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiawenc@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 <xiawenc@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
On 08/01/2012 06:49 AM, Stefan Hajnoczi wrote: > On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiawenc@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. >> >> +++ 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. Actually, LGPLv2+ (or compatible, like BSD), if you plan on making this a reusable library. GPLv2+ is too strict for libvirt to use directly.
On Wed, Aug 1, 2012 at 9:09 AM, Wenchao Xia <xiawenc@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 <xiawenc@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 > > 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 <stdarg.h> > +#include <stdio.h> > + > + > +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'. > +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 <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 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 > +{ > + 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 > +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) > +{ > + 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. > + 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. > +}; > + > +struct QBlockCreateOption { > + char *filename; > + char *fmt; > + char *base_filename; > + char *base_fmt; > + char *options; What would these be? > + 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; > + 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. > + 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. > +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 > > >
于 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 <xiawenc@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 >> >> 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 <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; >> + 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 <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 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 >
于 2012-8-1 20:49, Stefan Hajnoczi 写道: > On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiawenc@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 <xiawenc@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. > 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 <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? :) > 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 <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. > 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 >
于 2012-8-2 2:04, Blue Swirl 写道: > On Wed, Aug 1, 2012 at 9:09 AM, Wenchao Xia <xiawenc@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 <xiawenc@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 >> >> 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 <stdarg.h> >> +#include <stdio.h> >> + >> + >> +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 <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 > > 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 >> >> >> >
Il 02/08/2012 09:57, Wenchao Xia ha scritto: >>> +/* 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? g_strdup aborts on OOM. >>> +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. The question is *how* can it fail? It's a memset, do you envision it becoming something else? It's not about the performance of the processor, it's about the performance of whoever has to read the documentation and will wonder what the return code is for. (Though I proposed getting rid of this function too). >> 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. You can use offsetof instead of explicit checks: if (qpoo->struct_size >= offsetof(QBlockOpenOption, field)) /* use qpoo->field */ } or alternatively, something like this can be used to "complete" the struct with default values: QBlockOpenOption myopts; myopts.optional_field_1 = DEFAULT_VALUE_1; myopts.optional_field_2 = DEFAULT_VALUE_2; memcpy(&myopts, qpoo, qpoo->struct_size); myopts.struct_size = sizeof(myopts); > 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. What do you do if it doesn't match? >>> + 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. Yes, qb_create is fine. I was asking about qb_delete only. >> 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. The user is using C, he's supposed to know about memory allocation. Paolo
Il 02/08/2012 10:32, Wenchao Xia ha scritto: >> >> 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. Also assert that start and len are aligned. Paolo
On Thu, Aug 02, 2012 at 04:18:55PM +0800, Wenchao Xia wrote: > 于 2012-8-1 20:49, Stefan Hajnoczi 写道: > >On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiawenc@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 <xiawenc@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. > > > Certainly comments would be added. Great. Even at this early stage the doc comments will communicate your intentions to code reviewers and I think that helps. > >>+ > >>+#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? :) > > > 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. Yes, please don't use Hungarian notation. > >>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. > > > 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. Eric pointed out that we cannot use g_strdup() because it aborts on memory allocation failure, so please ignore my suggestion to use that. If you want to write a helper please make it a function (can be static inline) and avoid macros. Most of the time you probably don't need to free dest (and if you do, there's no need to test it for NULL before calling free(3)). Something like this isn't too long to open code: dest = strdup(src); if (dest == NULL) { goto err; } Using a helper function won't make this much shorter. > >>+ } > >>+ 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. I don't understand why there should be a difference between the two. They all take QBlockState* as the first argument. In an object-oriented language they would all be methods of the QBlockState class. > >>+ > >>+ 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. How about getting rid of QB_ERR_* and returning -errno from every libqblock API? Stefan
On Wed, Aug 01, 2012 at 07:25:54AM -0600, Eric Blake wrote: > On 08/01/2012 06:49 AM, Stefan Hajnoczi wrote: > > On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiawenc@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. > >> > >> +++ 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. > > Actually, LGPLv2+ (or compatible, like BSD), if you plan on making this > a reusable library. GPLv2+ is too strict for libvirt to use directly. Good point, I was only looking at libqblock-test.c but for the header file and library could we definitely need LGPLv2+. It makes sense for the whole thing to be LGPLv2+. Stefan
On Thu, Aug 02, 2012 at 10:59:20AM +0200, Paolo Bonzini wrote: > Il 02/08/2012 09:57, Wenchao Xia ha scritto: > >>> +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. > > The question is *how* can it fail? It's a memset, do you envision it > becoming something else? > > It's not about the performance of the processor, it's about the > performance of whoever has to read the documentation and will wonder > what the return code is for. > > (Though I proposed getting rid of this function too). Yes, please. Drop _init()/_uninit() and do this in _open()/_close(). I'm also not sure why qb_create() needs a QBlockState* argument. Can it be dropped? Stefan
Il 02/08/2012 12:17, Stefan Hajnoczi ha scritto: >> > 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. > Eric pointed out that we cannot use g_strdup() because it aborts on > memory allocation failure, so please ignore my suggestion to use that. I wouldn't. What good it is to avoid g_strdup, if the block layer will later use g_malloc anyway? Paolo
On Wed, Aug 01, 2012 at 07:25:54AM -0600, Eric Blake wrote: > On 08/01/2012 06:49 AM, Stefan Hajnoczi wrote: > > On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <xiawenc@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. > >> > >> +++ 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. > > Actually, LGPLv2+ (or compatible, like BSD), if you plan on making this > a reusable library. GPLv2+ is too strict for libvirt to use directly. NB, i don't see libvirt being able to use this library regardless of license, due to its reliance on glib + abort-on-OOM behaviour Daniel
Il 02/08/2012 13:11, Daniel P. Berrange ha scritto: >>> > > Please include GPLv2+ license headers in new source files you create. >>> > > See existing code like include/qemu/object.h for the license header >>> > > text. >> > >> > Actually, LGPLv2+ (or compatible, like BSD), if you plan on making this >> > a reusable library. GPLv2+ is too strict for libvirt to use directly. > NB, i don't see libvirt being able to use this library regardless > of license, due to its reliance on glib + abort-on-OOM behaviour Regarding glib, perhaps we could provide a small wrapper with the few functions we actually use. Paolo
于 2012-8-2 19:13, Paolo Bonzini 写道: > Il 02/08/2012 13:11, Daniel P. Berrange ha scritto: >>>>>> Please include GPLv2+ license headers in new source files you create. >>>>>> See existing code like include/qemu/object.h for the license header >>>>>> text. >>>> >>>> Actually, LGPLv2+ (or compatible, like BSD), if you plan on making this >>>> a reusable library. GPLv2+ is too strict for libvirt to use directly. >> NB, i don't see libvirt being able to use this library regardless >> of license, due to its reliance on glib + abort-on-OOM behaviour > > Regarding glib, perhaps we could provide a small wrapper with the few > functions we actually use. > That is what I expect, replace g_malloc and g_free in the library, and reports OOM. This may changes some code in qemu block layer. > Paolo >
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 <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; + 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 <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); \ + } \ + } \ +} + +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) +{ + 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; +} + +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; +}; + +struct QBlockCreateOption { + char *filename; + char *fmt; + char *base_filename; + char *base_fmt; + char *options; + 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); + +/* information */ +struct QBlockInfo { + /* 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; + 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); +int qbi_print_test(struct QBlockInfo *info); +int qb_getinfo(struct QBlockState *qbs, struct QBlockInfo *info); +#endif
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 <xiawenc@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