Re: [Qemu-devel] [PATCH] [RFC] libqblock draft code v1

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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
>>
>>
>>
>


-- 
Best Regards

Wenchao Xia