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

[Paolo Bonzini]

Il 02/08/2012 09:57, Wenchao Xia ha scritto:
>>> +/* 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.
>>
>   The macro was used to take care of OOM in every strdup and report
> that in returns, could g_strdup do that?

g_strdup aborts on OOM.


>>> +int qbi_init(struct QBlockInfo *info)
>>
>> Should return void.
>>
>   yes, it is useless now, but maybe in future it may fail. Returning
> int for every API results in only one more "eax store", I hope I can
> keep all API returns indicate success/fail, while add comments to say
> which API would not fail now.

The question is *how* can it fail?  It's a memset, do you envision it
becoming something else?

It's not about the performance of the processor, it's about the
performance of whoever has to read the documentation and will wonder
what the return code is for.

(Though I proposed getting rid of this function too).

>> 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).
>>
>   Seems a good way to make it compatiable, but I am worried how to
> make the checking logical simple for every structure. for eg:
> if (sizeof(QBlockOpenOption) != qpoo->struct_size) {
>     if (qpoo->struct_size == 32) {
>         /* Verion 1 */
>     } elseif (qpoo->struct_size == 40) {
>         /* Verion 2 */
>     }
>     .....
> }
>   This seems complicated.

You can use offsetof instead of explicit checks:

    if (qpoo->struct_size >= offsetof(QBlockOpenOption, field))
        /* use qpoo->field */
    }

or alternatively, something like this can be used to "complete" the
struct with default values:

    QBlockOpenOption myopts;
    myopts.optional_field_1 = DEFAULT_VALUE_1;
    myopts.optional_field_2 = DEFAULT_VALUE_2;
    memcpy(&myopts, qpoo, qpoo->struct_size);
    myopts.struct_size = sizeof(myopts);

>    Instead we can keep some space in the structureas:
> struct QBlockOpenOption {
>     const char *filename;
>     const char *fmt;
>     uint32 reserved[32]
> };
>     And make sure the structure size do not change at minor version
> change, then in library init:
> int libqblock_init(int verson)
> to check if user is using a compatiable version's header files.

What do you do if it doesn't match?


>>> +    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?
>   need a way to create new image, merge this function into open
> makes the option in qb_open a little strange, many option not used in
> normal open should be present in it, so I splitted the API out.

Yes, qb_create is fine.  I was asking about qb_delete only.

>> 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.
>>
>   that will forces an allocation on heap, user need to:
> info = qb_getinfo();
> qbi_free(info);
>   all parameters are freed and info itself is freed, but info still have
> invalid pointer value,so qbi_init/qbi_uninit pairs seems better.

The user is using C, he's supposed to know about memory allocation.

Paolo