qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V2 1/6] libqblock API design
Date: Fri, 14 Sep 2012 10:03:03 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120907 Thunderbird/15.0.1

Hi,
  about the OOM issue, I plan to drop the OOM request for now in
implemention, and at next step use rpc to wrap these APIs, then
user can select to link directly against the library or the wrapped
API, what do you think about it?
> 于 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 <address@hidden>
>>> ---
>>>   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   <address@hidden>
>>> + *
>>> + * 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]