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

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