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

[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 <address@hidden>
> ---
>  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