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

[Wenchao Xia]

于 2012-8-1 20:49, Stefan Hajnoczi 写道:
> On Wed, Aug 1, 2012 at 10:09 AM, Wenchao Xia <address@hidden> 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 <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
>
> 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


--
Best Regards

Wenchao Xia