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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2012-8-14 4:00, Blue Swirl 写道:
> On Mon, Aug 13, 2012 at 11:27 AM, Wenchao Xia
> <xiawenc@linux.vnet.ibm.com> wrote:
>> 于 2012-8-11 20:18, Blue Swirl 写道:
>>
>>> On Fri, Aug 10, 2012 at 8:04 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> 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
>>>>> <xiawenc@linux.vnet.ibm.com>
>>>>> 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).

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