Re: [Qemu-devel] [PATCH V18 08/10] libqblock: libqblock API implement

[Wenchao Xia]

> On Sat, Feb 9, 2013 at 7:42 AM, Wenchao Xia <address@hidden> wrote:
> >    This patch contains implemention for APIs. Basically it is a layer
> > above qemu block general layer now.
> >    qb_image_new() will try do init for this library.
> >
> > Signed-off-by: Wenchao Xia <address@hidden>
> > ---
> >   libqblock/libqblock-error.c |   49 +++
> >   libqblock/libqblock.c       |  991 +++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 1040 insertions(+), 0 deletions(-)
> >
> > diff --git a/libqblock/libqblock-error.c b/libqblock/libqblock-error.c
> > index e69de29..2367ab4 100644
> > --- a/libqblock/libqblock-error.c
> > +++ b/libqblock/libqblock-error.c
> > @@ -0,0 +1,49 @@

> > +
> > +typedef struct LibqbFormatStrMapping {
> > +    const char *fmt_str;
> > +    QBlockFormat fmt_type;
> > +} LibqbFormatStrMapping;
> > +
> > +LibqbFormatStrMapping libqb_formatstr_table[] = {
>
> static const
  OK.

> > +    {"cow", QB_FORMAT_COW},
> > +    {"qed", QB_FORMAT_QED},
> > +    {"qcow", QB_FORMAT_QCOW},
> > +    {"qcow2", QB_FORMAT_QCOW2},
> > +    {"raw", QB_FORMAT_RAW},
> > +    {"rbd", QB_FORMAT_RBD},
> > +    {"sheepdog", QB_FORMAT_SHEEPDOG},
> > +    {"vdi", QB_FORMAT_VDI},
> > +    {"vmdk", QB_FORMAT_VMDK},
> > +    {"vpc", QB_FORMAT_VPC},
> > +    {NULL, 0},
>
> You can avoid this NULL entry and save space by using ARRAY_SIZE() in
> the loop. Compiler could also unroll the loop.
  OK.

> > +};
> > +
> > +__attribute__((constructor))
> > +static void libqblock_init(void)
> > +{
> > +    /* Todo: add an assertion about the ABI. */
> > +    libqb_global_data.init_flag = 0;
> > +    pthread_mutex_init(&libqb_global_data.mutex, NULL);
> > +}
> > +
> > +const char *qb_formattype2str(QBlockFormat fmt_type)
> > +{
> > +    int i = 0;
> > +    LibqbFormatStrMapping *tb = libqb_formatstr_table;
>
> This does not seem to be useful, you could use libqb_formatstr_table directly.
  OK.

> > +
> > +    if ((fmt_type <= QB_FORMAT_NONE) || (fmt_type >= QB_FORMAT_MAX)) {
> > +        return NULL;
> > +    }
> > +    while (tb[i].fmt_str != NULL) {
>
> for (i = 0; i < ARRAY_SIZE(libqb_formatstr_table); i++) {
  OK.

> > +        if (tb[i].fmt_type == fmt_type) {
> > +            return tb[i].fmt_str;
> > +        }
> > +        i++;
> > +    }
> > +    return NULL;
> > +}
> > +
> > +QBlockFormat qb_str2fmttype(const char *fmt_str)
> > +{
> > +    int i = 0;
> > +    LibqbFormatStrMapping *tb = libqb_formatstr_table;
> > +
> > +    if (fmt_str == NULL) {
> > +        return QB_FORMAT_NONE;
> > +    }
> > +    while (tb[i].fmt_str != NULL) {
> > +        if ((strcmp(fmt_str, tb[i].fmt_str) == 0)) {
> > +            return tb[i].fmt_type;
> > +        }
> > +        i++;
> > +    }
> > +    return QB_FORMAT_NONE;
> > +}
> > +
> > +static void set_context_err(QBlockContext *context, int err_ret,
> > +                            const char *fmt, ...)
>
> GCC_FMT_ATTR()?
  Do you mean this declaration is needed here?

> > +{
> > +    va_list ap;
> > +
> > +    if (context->g_error != NULL) {
> > +        g_error_free(context->g_error);
> > +    }
> > +
> > +    va_start(ap, fmt);
> > +    context->g_error = g_error_new_valist(qb_error_quark(), err_ret, fmt, ap);
> > +    va_end(ap);
> > +
> > +    context->err_ret = err_ret;
> > +    if (err_ret == QB_ERR_INTERNAL_ERR) {
> > +        context->err_no = -errno;
> > +    } else {
> > +        context->err_no = 0;
> > +    }
> > +}
> > +

> > +static void delete_context(QBlockContext **p_context)
> > +{
> > +    if ((*p_context)->g_error != NULL) {
> > +        g_error_free((*p_context)->g_error);
> > +    }
> > +    g_free(*p_context);
> > +    *p_context = NULL;
> > +    return;
>
> Useless return, please remove.
  OK.


> > --
> > 1.7.1


--
Best Regards

Wenchao Xia