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

[Stefan Hajnoczi]

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

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