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

[Blue Swirl]

On Wed, Feb 13, 2013 at 2:08 AM, Wenchao Xia <address@hidden> wrote:
>
>> 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?

It declares to GCC that this function uses printf like format strings,
so it can perform additional checks. This helps catching bugs by
callers supplying incorrect arguments.

>
>
>>> +{
>>> +    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
>