Re: [Qemu-devel] [PATCH V2 1/6] libqblock API design

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

于 2012-9-11 5:07, Eric Blake 写道:
> On 09/10/2012 02:26 AM, Wenchao Xia wrote:
>>    This patch contains the major APIs in the library.
>> Important APIs:
>>    1 QBroker. These structure was used to retrieve errors, every thread must
>> create one first, later maybe thread related staff could be added into it.
>>    2 QBlockState. It stands for an block image object.
>>    3 QBlockStaticInfo. It contains static information such as location, backing
>> file, size.
>>    4 ABI was kept with reserved members.
>>    5 Sync I/O. It is similar to C file open, read, write and close operations.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   libqblock/libqblock.c | 1077 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   libqblock/libqblock.h |  292 +++++++++++++
>
> Two new files, but no build machinery to build them to see if they have
> blatant errors.  Yes, it is bisectable, but no, the bisection is not
> useful.  I'd rather see the Makefile patches with stub files at the
> beginning, then flesh out the stubs, where each patch builds along the way.
>
> In particular,
>
   OK, I understand the importance to make each patch workable to
maintainer, will adjust the patches to make each one works.

>> +++ b/libqblock/libqblock.c
>
>> +#include "libqblock.h"
>> +#include "libqblock-internal.h"
>
> There is no libqblock-internal.h with just this patch applied, making it
> impossible to validate this patch in order (the fact that 'make' isn't
> flagging this incomplete patch is because you aren't building
> libqblock-o yet).  I'm planning on overlooking the .c file and focusing
> on the user interface (I'd rather leave the detailed review to those
> more familiar with qemu, while I'm personally worried about how libvirt
> would ever use libqblock if you ever solved the glib-aborts-on-OOM issue
> to make the library even worth using).
>
   About the OOM issue, I think there are potential 3 ways to solve it:
1 modify qemu block layer code, in this way providing libqblock at first
will help, for that it will encapsulate and isolate all codes needed
for block layer, and we got test case on the top to validate OOM
behavior.
2 Using glib and forgot OOM now. Then at higher layer, they have two
choice: if they want no exiting on OOM, wrap the API with xmlrpc or
something like; otherwise directly use the API.
3 switch the implemention of libqblock, do not link qemu block code
directly, fork and execute qemu-img, qemu-nbd. This require a
re-implement about AIO in libqblock, with GSource AIO framework I am
not sure if it would exit on OOM, but I guess better to not involve any
glib in this case. Additional challenge would be, any more
functionalities adding require patch for qemu-img, qemu-io, qemu-nbd
first, such as image information retrieving, allocation detection,
and libqblock need to parse string output carefully, better to get
all output in json format. Personally I am worried to handle many
exception in string parsing.

   To me, the second way seems most reasonable, it allows qemu block
remain tightly bind to glib. The first way also make sense, but
need to loose the tight between qemu and glib. what do you think?

>> +++ b/libqblock/libqblock.h
>> @@ -0,0 +1,292 @@
>> +/*
>> + * QEMU block layer library
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + *  Wenchao Xia   <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef LIBQBLOCK_H
>> +#define LIBQBLOCK_H
>> +
>> +#include "libqblock-types.h"
>> +#include "libqblock-error.h"
>
> Even worse - you've introduced a public header that I'm supposed to be
> able to use, but I can't use it because libqblock-types.h and
> libqblock-error.h don't exist.  I'd much rather review a patch series
> built incrementally from ground up, with each piece working, rather than
> reviewing an API that depends on constructs that aren't defined until
> later patches.
>
>> +/**
>> + * qb_broker_new: allocate a new broker
>> + *
>> + * Broker is used to pass operation to libqblock, and get feed back from it.
>
> s/feed back/feedback/
>
>> +/**
>> + * qb_state_delete: free a QBlockState struct
>> + *
>> + * if image was opened, qb_close must be called before delete.
>
> And if it wasn't closed, what happens?  Should this function return int,
> instead of void, to error out in the case that it is called out of order?
>
>> +/**
>> + * qb_prot_info_new: create a new struct QBlockProtInfo.
>
> Inconsistent on whether your descriptions end in '.' or not.
>
>> +/* sync access */
>> +/**
>> + * qb_read: block sync read.
>> + *
>> + * return number of bytes read, libqblock negative error value on fail.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @buf: buffer that receive the content.
>> + * @len: length to read.
>> + * @offset: offset in the block data.
>> + */
>> +DLL_PUBLIC
>> +int64_t qb_read(struct QBroker *broker,
>> +                struct QBlockState *qbs,
>> +                uint8_t *buf,
>> +                uint32_t len,
>> +                uint64_t offset);
>
> Seems odd to have 32 bit limit on input, when output handles 64 bit.
>
>> +
>> +/**
>> + * qb_write: block sync write.
>> + *
>> + * return number of bytes written, libqblock negative error value on fail.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @buf: buffer that receive the content.
>> + * @len: length to write.
>> + * @offset: offset in the block data.
>> + */
>> +DLL_PUBLIC
>> +int64_t qb_write(struct QBroker *broker,
>> +                 struct QBlockState *qbs,
>> +                 const uint8_t *buf,
>> +                 uint32_t len,
>> +                 uint64_t offset);
>
> and again.
>
>> +/* advance image APIs */
>> +/**
>> + * qb_check_allocation: check if [start, start+lenth-1] was allocated on the
>> + *  image.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @start: start position, unit is byte.
>> + * @length: length to check, unit is byte.
>
> Needs to be at least int32_t to match your read/write; or better yet
> int64_t to allow probes of more than 2G at a time.
>
>> + * @pstatus: pointer to receive the status, 1 means allocated,
>> + *  0 means unallocated.
>> + * @plength: pointer to receive the length that all have the same status as
>> + *  *pstatus.
>> + *
>> + * Note: after return, start+*plength may have the same status as
>> + *  start+*plength-1.
>> + */
>> +DLL_PUBLIC
>> +int qb_check_allocation(struct QBroker *broker,
>> +                        struct QBlockState *qbs,
>> +                        uint64_t start,
>> +                        int length,
>> +                        int *pstatus,
>> +                        int *plength);
>
> If you change the type of length, then plength needs to match.
>
>> +/**
>> + * qb_fmttype2str: libqblock format enum type to a string.
>> + *
>> + * return a pointer to the string, or NULL if type is not supported, and
>> + *  returned pointer do NOT need to free.
>
> grammar; I suggest:
> returned pointer must not be freed
>


-- 
Best Regards

Wenchao Xia