Re: [Qemu-devel] [qemu-devel] [PATCH V2 0/3] [RFC] libqblock draft code v2

[Blue Swirl]

On Tue, Aug 14, 2012 at 8:52 AM, Wenchao Xia <address@hidden> wrote:
> 于 2012-8-14 4:00, Blue Swirl 写道:
>
>> On Mon, Aug 13, 2012 at 11:27 AM, Wenchao Xia
>> <address@hidden> wrote:
>>>
>>> 于 2012-8-11 20:18, Blue Swirl 写道:
>>>
>>>> On Fri, Aug 10, 2012 at 8:04 AM, Wenchao Xia
>>>> <address@hidden>
>>>> wrote:
>>>>>
>>>>>
>>>>>      Thanks for your review, sorry I have forgot some fixing you
>>>>> mentioned before, will correct them this time.
>>>>>
>>>>> 于 2012-8-10 1:12, Blue Swirl 写道:
>>>>>
>>>>>> On Thu, Aug 9, 2012 at 10:10 AM, Wenchao Xia
>>>>>> <address@hidden>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>      This patch intrudce libqblock API, libqblock-test is used as a
>>>>>>> test
>>>>>>> case.
>>>>>>>
>>>>>>> V2:
>>>>>>>      Using struct_size and [xxx]_new [xxx]_free to keep ABI.
>>>>>>>      Using embbed structure to class format options in image
>>>>>>> creating.
>>>>>>>      Format specific options were brought to surface.
>>>>>>>      Image format was changed to enum interger instead of string.
>>>>>>>      Some API were renamed.
>>>>>>>      Internel error with errno was saved and with an API caller can
>>>>>>> get
>>>>>>> it.
>>>>>>>      ALL flags used were defined in libqblock.h.
>>>>>>>
>>>>>>> Something need discuss:
>>>>>>>      Embbed structure or union could make the model more friendly,
>>>>>>> but
>>>>>>> that
>>>>>>> make ABI more difficult, because we need to check every embbed
>>>>>>> structure's
>>>>>>> size and guess compiler's memory arrangement. This means #pragma
>>>>>>> pack(4)
>>>>>>> or struct_size, offset_next in every structure. Any better way to
>>>>>>> solve
>>>>>>> it?
>>>>>>> or make every structure a plain one?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'd still use accessor functions instead of structure passing, it
>>>>>> would avoid these problems.
>>>>>>
>>>>>     Do you mean some function like :
>>>>>     CreateOption_Filename_Set(const char *filename)
>>>>>     CreateOption_Format_Set(const char *filename)
>>>>
>>>>
>>>>
>>>> Something like this:
>>>> int qb_create_cow(struct QBlockState *qbs, const char *filename,
>>>> size_t virt_size, const char *backing_file, int backing_flag);
>>>> etc. for rest of the formats.
>>>>
>>>> Alternatively, we could have more generic versions like you describe,
>>>> or even more generic still:
>>>>
>>>> void qb_set_property(struct QBlockState *qbs, const char *prop_name,
>>>> const char *prop_value);
>>>>
>>>> so the create sequence (ignoring error handling) would be:
>>>> s = qb_init();
>>>> qb_set_property(s, "filename", "c:\\\\autoexec.bat");
>>>> qb_set_property(s, "format", "cow");
>>>> qb_set_property(s, "virt_size", "10GB");
>>>> //  use defaults for backing_file and backing_flag
>>>> qb_create(s);
>>>>
>>>> Likewise for info structure:
>>>> char *qb_get_property(struct QBlockState *qbs, const char *prop_name);
>>>>
>>>> foo = qb_get_property(s, "format");
>>>> foo = qb_get_property(s, "encrypted");
>>>> foo = qb_get_property(s, "num_backing_files");
>>>> foo = qb_get_property(s, "virt_size");
>>>>
>>>> This would be helpful for the client to display parameters without
>>>> much understanding of their contents:
>>>> char **qb_list_properties(struct QBlockState *qbs); /* returns array
>>>> of property names available for this file, use get_property to
>>>> retrieve their contents */
>>>>
>>>> But the clients can't be completely ignorant of all formats available,
>>>> for example a second UI dialog needs to be added for formats with
>>>> backing files, otherwise it won't be able to access some files at all.
>>>> Maybe by adding type descriptions for each property (type for
>>>> "filename" is "path", for others "string", "bool", "enum" etc).
>>>>
>>>    Thanks. This seems heavy document is needed for that no structure
>>> can indicate what options sub format have, user can only get that info
>>> from returns or documents. I am not sure if this is good, because it
>>> looks more like a object oriented API that C.
>>
>>
>> This approach may be a bit over-engineered, but it may be simpler to
>> tie to an UI.
>>
>> What do you think of the simple version then:
>> int qb_create_cow(struct QBlockState *qbs, const char *filename,
>> size_t virt_size, const char *backing_file, int backing_flag);
>>
>   it is hard to extend more options, if for every format a API is needed,
> then
> int qb_create_cow(struct QBlockState *qbs, struct QBlockOptionFmtCow *op)
> seems better, while keep struct QBlockOptionFmtCow a plain struct
> without embbed struct(using pointer instead).

OK,  practically there isn't much difference and if a lot of
parameters are needed, a structure is needed anyway.

>
>
>>>
>>>
>>>
>>>>>
>>>>>     It can solve the issue, with a cost of more small APIs in header
>>>>> files that user should use. Not sure if there is a good way to make
>>>>> it more friendly as an object language:
>>>>>     "oc.filename = name;" automatically call CreateOption_Filename_Set,
>>>>> API CreateOption_Filename_Set is invisible to user.
>>>>>
>>>>>
>>>>>
>>>>>> Packing can even introduce a new set of problems since we don't
>>>>>> control the CFLAGS of the client of the library.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>     indeed, I tried to handle the difference in function
>>>>> qboo_adjust_o2n,
>>>>> found that structure member alignment is hard to deal.
>>>>>
>>>>>
>>>>>>>      AIO is missing, need a prototype.
>>>>>>>
>>>>>>> Wenchao Xia (3):
>>>>>>>      adding libqblock
>>>>>>>      libqblock API
>>>>>>>      libqblock test case
>>>>>>>
>>>>>>>     Makefile         |    3 +
>>>>>>>     block.c          |    2 +-
>>>>>>>     block.h          |    1 +
>>>>>>>     libqblock-test.c |  197 ++++++++++++++++
>>>>>>>     libqblock.c      |  670
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     libqblock.h      |  447 ++++++++++++++++++++++++++++++++++++
>>>>>>>     6 files changed, 1319 insertions(+), 1 deletions(-)
>>>>>>>     create mode 100644 libqblock-test.c
>>>>>>>     create mode 100644 libqblock.c
>>>>>>>     create mode 100644 libqblock.h
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best Regards
>>>>>
>>>>> Wenchao Xia
>>>>>
>>>>
>>>
>>>
>>> --
>>> Best Regards
>>>
>>> Wenchao Xia
>>>
>>
>
>
> --
> Best Regards
>
> Wenchao Xia
>