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 <xiaw...@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, > +++ 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). > +++ b/libqblock/libqblock.h > @@ -0,0 +1,292 @@ > +/* > + * QEMU block layer library > + * > + * Copyright IBM, Corp. 2012 > + * > + * Authors: > + * Wenchao Xia <xiaw...@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 -- Eric Blake ebl...@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
