Re: [Qemu-devel] [PATCH 1/6] libqblock APIs

[Eric Blake]

On 09/03/2012 03:18 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 QBlockInfoImageStatic. Now it is not folded with location and format.
>   4 ABI was kept with reserved members.
> 
> structure, because it would cause caller more codes to find one member.

Commit message snafu?

> +/**
> + * libqblock_init: Initialize the library
> + */
> +void libqblock_init(void);

Is this function safe to call more than once?  Even tighter, is it safe
to call this function simultaneously from multiple threads?

> +
> +/**
> + * qb_broker_new: allocate a new broker
> + *
> + * Broker is used to pass operation to libqblock, and got feed back from it.

s/got feed back/get feedback/

> + *
> + * Returns 0 on success, negative value on fail.

Is there any particular interpretation to this negative value, such as
negative errno constant, or always -1?

> +
> +/**
> + * qb_state_new: allocate a new QBloctState struct

s/Bloct/Block/

> + *
> + * Following qblock action were based on this struct

Didn't read well.  Did you mean:

Subsequent qblock actions will use this struct

> + *
> + * Returns 0 if succeed, negative value on fail.

Again, is there any particular meaning to which negative value is returned?

> +
> +/**
> + * qb_open: open a block object.
> + *
> + * return 0 on success, negative on fail.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @loc: location options for open, how to find the image.
> + * @fmt: format options, how to extract the data, only valid member now is
> +     fmt->fmt_type, set NULL if you want auto discovery the format.

set to NULL if you want to auto-discover the format

Maybe also add a warning about the inherent security risks of attempting
format auto-discovery (any raw image must NOT be probed, as the raw
image can emulate any other format and cause qemu to chase down chains
where it should not).

> + * @flag: behavior control flags.

What flags are currently defined?

> + */
> +int qb_open(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            struct QBlockOptionLoc *loc,
> +            struct QBlockOptionFormat *fmt,
> +            int flag);
> +
> +/**
> + * qb_close: close a block object.
> + *
> + * qb_flush is automaticlly done inside.

s/automaticlly/automatically/

> +/**
> + * qb_create: create a block image or object.
> + *
> + * Note: Create operation would not open the image automatically.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @loc: location options for open, how to find the image.
> + * @fmt: format options, how to extract the data.
> + * @flag: behavior control flags.

Again, what flags are defined?

> +
> +/* sync access */
> +/**
> + * qb_read: block sync read.
> + *
> + * return negative on fail, 0 on success.

Shouldn't this instead return the number of successfully read bytes, to
allow for short reads if offset exceeds end-of-file?  If so, should it
return ssize_t instead of int?

> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @buf: buffer that receive the content.

s/receive/receives/

> + * @len: length to read.

Is there a magic length for reading the entire file in one go?

> + * @offset: offset in the block data.
> + */
> +int qb_read(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            const void *buf,
> +            size_t len,
> +            off_t offset);

You do realize that size_t and off_t are not necessarily the same width;
but I'm okay with limiting to size_t reads.

> +/**
> + * qb_write: block sync write.
> + *
> + * return negative on fail, 0 on success.

Again, this should probably return number of successfully written bytes,
as an ssize_t.

> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @buf: buffer that receive the content.

s/receive/supplies/

> +/* advance image APIs */
> +/**
> + * qb_is_allocated: check if following sectors was allocated on the image.
> + *
> + * return negative on fail, 0 or 1 on success. 0 means unallocated, 1 means
> + *allocated.

Formatting is off.

> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @sector_num: start sector number. If 'sector_num' is beyond the end of the
> + *disk image the return value is 0 and 'pnum' is set to 0.
> + * @nb_sectors: how many sector would be checked, it is the max value 'pnum'
> + *should be set to.  If nb_sectors goes beyond the end of the disk image it
> + *will be clamped.
> + * @pnum: pointer to receive how many sectors are allocated or unallocated.

This interface requires the user to know how big a sector is.  Would it
be any more convenient to the user to pass offsets, rather than sector
numbers; and/or have a function for converting between offsets and
sector numbers?

> + */
> +int qb_is_allocated(struct QBroker *broker,
> +                    struct QBlockState *qbs,
> +                    int64_t sector_num,
> +                    int nb_sectors,

Shouldn't nb_sectors be size_t?

> +                    int *pnum);

Exactly how does the *pnum argument work?  This interface looks like it
isn't fully thought out yet.  Either I want to know if a chunk of
sectors is allocated (I supply start and length of sectors to check),
regardless of how many sectors beyond that point are also allocated
(pnum makes no sense); or I want to know how many sectors are allocated
from a given point (I supply start, and the function returns length, so
nb_sectors makes no sense).  Either way, I think you are supplying too
many parameters for how I envision checking for allocated sectors.

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature