Re: [Qemu-devel] [PATCH V2 2/6] libqblock type and structure defines

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]

On 09/10/2012 02:26 AM, Wenchao Xia wrote:
>   This patch contains type and defines used in APIs, one file for public usage
> by user, one for libqblock internal usage.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  libqblock/libqblock-internal.h |   50 ++++++++
>  libqblock/libqblock-types.h    |  251 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 301 insertions(+), 0 deletions(-)
>  create mode 100644 libqblock/libqblock-internal.h
>  create mode 100644 libqblock/libqblock-types.h
> 

As mentioned in 1/6, this should come earlier in the series, as it lays
the fundamentals used by other new files.

> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h
> new file mode 100644
> index 0000000..fa27ed4
> --- /dev/null
> +++ b/libqblock/libqblock-internal.h
> @@ -0,0 +1,50 @@

> +
> +#define QB_ERR_STRING_SIZE (1024)
> +struct QBroker {
> +    /* last error */
> +    char err_msg[QB_ERR_STRING_SIZE];

Is this fixed-width struct going to bite us in the future?  Suppose I
pass in a file name that is already 1000 bytes long; it seems like I
might be able to get you to overflow this buffer if your error message
includes the name of my offending file.

> +++ b/libqblock/libqblock-types.h
> +/* this library is designed around this core struct. */
> +struct QBlockState;
> +
> +/* every thread would have a broker. */

s/would/should/

> +
> +/**
> + * QBlockStaticInfo: information about the block image.
> + *
> + * @loc: location info.
> + * @fmt_type: format type.
> + * @virt_size: virtual size in bytes.
> + * @backing_loc: backing file location, its type is QB_PROT_NONE if not exist.
> + * @encrypt: encrypt flag.
> + * @sector_size: how many bytes in a sector, it is 512 usually.
> + */
> +struct QBlockStaticInfo {
> +    struct QBlockProtInfo loc;
> +    enum QBlockFmtType fmt_type;
> +    uint64_t virt_size;
> +    /* advance info */
> +    struct QBlockProtInfo backing_loc;
> +    bool encrypt;
> +    int sector_size;
> +};

No reserved space for potential growth in this structure?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]