Re: [Qemu-devel] [PATCH V19 07/10] libqblock: libqblock API design and type defines

[Eric Blake]

On 02/15/2013 07:48 PM, Wenchao Xia wrote:
>   Public API design header files: libqblock.h, libqblock-error.h.
> Public type define header files: libqblock-types.h. Private internal used
> header files: libqblock-internal. For ABI some reserved bytes are used in
> structure defines. Macro QEMU_DLL_PUBLIC is used to mark exported function.
> 
> Important APIs:
>   1 QBlockImage. It stands for an block image object.
>   2 QBlockStaticInfo. It contains static information such as location, backing
> file, size.
>   3 Sync I/O. It is similar to C file open, read, write and close operations.
> 
> + */
> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512/sizeof(QBlockReserved))
> +typedef struct QBlockLocationInfo {
> +    QBlockProtocol prot_type;
> +    union {
> +        QBlockProtocolOptionsFile o_file;
> +        QBlockReserved union_reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE];
> +    };
> +    QBlockReserved reserved[LIBQBLOCK_RESERVED_SIZE];

Do you really mean to reserve trailing bytes after what you have already
reserved as part of the union?  Theoretically, the reserved space in the
union seems like it would be enough.


> +
> +typedef struct QBlockFormatOptionsQCOW2 {
> +    QBlockLocationInfo backing_loc;
> +    QBlockFormat backing_fmt;
> +    bool encrypt;
> +    uint64_t cluster_size; /* unit is bytes */
> +    QBlockFormatOptionsQCOW2CompatLv cpt_lv;
> +    QBlockFormatOptionsQCOW2PreAlloc pre_mode;
> +    QBlockReserved reserved[LIBQBLOCK_RESERVED_SIZE];
> +} QBlockFormatOptionsQCOW2;

Side note: Libvirt is leaning towards presenting compat-level 1.1 to the
user via the name 'qcow3', than it is to try and teach the users about
the difference between qcow2 (compat 1.0) and qcow2v3 (compat 1.1) all
under the name qcow2.  Even though the qcow2 code is able to handle both
compatibility levels under the hood in qemu source code, it seems less
confusing if the user interface separates things with a 'qcow3' naming
convention.  I don't know how much that should impact your design, though.


> +
> +#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2)

Spaces around binary operators.


> +/**
> + * QBlockStaticInfo: information about the block image.
> + *
> + * @loc: location information.
> + * @fmt: format information.
> + * @sector_size: how many bytes in a sector, it is 512 usually.

I'd drop ', it is 512 usually', since in the future, we might want to
default to 4096 in new images.


> +/**
> + * qb_create: create a block image or object.
> + *
> + * Note: Create operation would not open the image automatically.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @qbi: pointer to QBlockImage, used to get error message.
> + * @loc: location options for open, how to find the image.
> + * @fmt: format options, how to extract the data.
> + * @flag: behavior control flags, LIBQBLOCK_O_XXX's combination, it is used 
> if
> + *  backing file need to be opened..

s/.././


> +
> +/* advance image APIs */
> +/**
> + * qb_check_allocation: check if [start, start+length-1] was allocated on the
> + *  image.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @qbi: pointer to QBlockImage.
> + * @start: start position, unit is byte.
> + * @length: length to check, unit is byte, max is (INTMAX-1)*sector_size. If 
> it

INT_MAX, not INTMAX.

Might be worth pointing to the API for determining sector_size, since it
is not a parameter in this function call.  Furthermore, since all the
relevant parameters are uint64_t, why is there an INT_MAX limitation in
the first place?  Even if underlying qemu code has a limitation at 2
gigabytes per iteration, can't the libqblock interface iterate as many
times as needed to reach the user's desired 64-bit length?

> + *   is too big, QB_ERR_INVALID_PARAM will be returned.
> + * @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.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_check_allocation(QBlockImage *qbi,
> +                        uint64_t start,
> +                        uint64_t length,
> +                        int *pstatus,
> +                        uint64_t *plength);

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature