Re: [Qemu-devel] [PATCH V4 2/5] libqblock type defines

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

   I will change the code for most of comments, but have some response
below.
> Il 27/09/2012 04:23, Wenchao Xia ha scritto:
>>    This patch contains type and macro 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 |   57 +++++++++
>>   libqblock/libqblock-types.h    |  268 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 325 insertions(+), 0 deletions(-)
>>   create mode 100644 libqblock/libqblock-internal.h
>>   create mode 100644 libqblock/libqblock-types.h
>>
>> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h
>> new file mode 100644
>> index 0000000..d3e983c
>> --- /dev/null
>> +++ b/libqblock/libqblock-internal.h
>> @@ -0,0 +1,57 @@
>> +/*
>> + * QEMU block layer library
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + *  Wenchao Xia   <xiawenc@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_INTERNAL
>> +#define LIBQBLOCK_INTERNAL
>> +
>> +#include "glib.h"
>> +
>> +#include "block.h"
>> +#include "block_int.h"
>> +#include "libqblock-types.h"
>> +
>> +/* this file contains defines and types used inside the library. */
>> +
>> +#define FUNC_FREE(p) g_free((p))
>> +#define FUNC_MALLOC(size) g_malloc((size))
>> +#define FUNC_CALLOC(nmemb, size) g_malloc0((nmemb)*(size))
>> +#define FUNC_STRDUP(p) g_strdup((p))
>> +
>> +#define CLEAN_FREE(p) { \
>> +        FUNC_FREE(p); \
>> +        (p) = NULL; \
>> +}
>> +
>> +/* details should be hidden to user */
>> +struct QBlockState {
>> +    BlockDriverState *bdrvs;
>> +    /* internal used file name now, if it is not NULL, it means
>> +       image was opened.
>> +    */
>> +    char *filename;
>> +};
>> +
>> +struct QBlockContext {
>> +    /* last error */
>> +    GError *g_error;
>> +    int err_ret; /* 1st level of error, the libqblock error number */
>> +    int err_no; /* 2nd level of error, errno what below reports */
>> +};
>> +
>> +#define G_LIBQBLOCK_ERROR g_libqbock_error_quark()
>> +
>> +static inline GQuark g_libqbock_error_quark(void)
>> +{
>> +    return g_quark_from_static_string("g-libqblock-error-quark");
>> +}
>> +#endif
>> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h
>> new file mode 100644
>> index 0000000..948ab8a
>> --- /dev/null
>> +++ b/libqblock/libqblock-types.h
>> @@ -0,0 +1,268 @@
>> +/*
>> + * QEMU block layer library
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + *  Wenchao Xia   <xiawenc@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_TYPES_H
>> +#define LIBQBLOCK_TYPES_H
>> +
>> +#include <sys/types.h>
>> +#include <stdint.h>
>> +#include <stdbool.h>
>> +
>> +#if defined(__GNUC__) && __GNUC__ >= 4
>> +    #ifdef LIBQB_BUILD
>> +        #define DLL_PUBLIC __attribute__((visibility("default")))
>> +    #else
>> +        #define DLL_PUBLIC
>> +    #endif
>> +#else
>> +    #warning : gcc compiler version < 4, symbols can not be hidden.
>
> You don't need to warn.  If LIBQB_BUILD is not defined, DLL_PUBLIC would
> be empty anyway.  If LIBQB_BUILD is defined, you know GCC >= 4.0 because
> you pass -fvisibility=hidden from the Makefile.
>
>> +#endif
>> +
>> +/* this library is designed around this core struct. */
>> +typedef struct QBlockState QBlockState;
>> +
>> +/* every thread should have a context. */
>> +typedef struct QBlockContext QBlockContext;
>> +
>> +/* flag used in open and create */
>> +#define LIBQBLOCK_O_RDWR        0x0002
>> +/* do not use the host page cache */
>> +#define LIBQBLOCK_O_NOCACHE     0x0020
>> +/* use write-back caching */
>> +#define LIBQBLOCK_O_CACHE_WB    0x0040
>> +/* don't open the backing file */
>> +#define LIBQBLOCK_O_NO_BACKING  0x0100
>> +/* disable flushing on this disk */
>> +#define LIBQBLOCK_O_NO_FLUSH    0x0200
>> +
>> +#define LIBQBLOCK_O_CACHE_MASK \
>> +   (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH)
>> +
>> +#define LIBQBLOCK_O_VALID_MASK \
>> +   (LIBQBLOCK_O_RDWR | LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | \
>> +    LIBQBLOCK_O_NO_BACKING | LIBQBLOCK_O_NO_FLUSH)
>> +
>> +typedef enum QBlockProtType {
>> +    QB_PROT_NONE = 0,
>> +    QB_PROT_FILE,
>> +    QB_PROT_MAX
>> +} QBlockProtType;
>
> Use QBlockProtocol, QB_PROTO_*.
>
>> +typedef struct QBlockProtOptionFile {
>> +    const char *filename;
>> +} QBlockProtOptionFile;
>
> Use QBlockProtocolOptionsFile
>
>> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512)
>> +typedef union QBlockProtOptionsUnion {
>> +    QBlockProtOptionFile o_file;
>> +    uint8_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE];
>> +} QBlockProtOptionsUnion;
>
> Not really options, because they are required.  I would just not name
> the union, and embed it in the struct.  Also please change it to
>
>       uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE / 8];
>
> (Sorry for not thinking about this before).  This will fix the alignment
> and ensure that future changes to the union do not change the ABI.
>
   You are right, I forgot alignment issue before.

>> +/**
>> + * struct QBlockProtInfo: contains information about how to find the image
>> + *
>> + * @prot_type: protocol type, now only support FILE.
>> + * @prot_op: protocol related options.
>> + */
>> +typedef struct QBlockProtInfo {
>> +    QBlockProtType prot_type;
>> +    QBlockProtOptionsUnion prot_op;
>> +} QBlockProtInfo;
>
> What about QBlockLocation instead?  Or QBlockLocationInfo.
>
>> +
>> +/* format related options */
>> +typedef enum QBlockFmtType {
>> +    QB_FMT_NONE = 0,
>> +    QB_FMT_COW,
>> +    QB_FMT_QED,
>> +    QB_FMT_QCOW,
>> +    QB_FMT_QCOW2,
>> +    QB_FMT_RAW,
>> +    QB_FMT_RBD,
>> +    QB_FMT_SHEEPDOG,
>> +    QB_FMT_VDI,
>> +    QB_FMT_VMDK,
>> +    QB_FMT_VPC,
>> +    QB_FMT_MAX
>> +} QBlockFmtType;
>
> QBlockFormat; similarly remove "Type" from all enums.
>
>> +typedef struct QBlockFmtOptionCow {
>> +    uint64_t virt_size;
>> +    QBlockProtInfo backing_loc;
>> +} QBlockFmtOptionCow;
>
> QBlockFormatOptionsCOW.  Similarly use the plural for all other formats.
>
>> +typedef struct QBlockFmtOptionQed {
>> +    uint64_t virt_size;
>> +    QBlockProtInfo backing_loc;
>> +    QBlockFmtType backing_fmt;
>> +    uint64_t cluster_size; /* unit is bytes */
>> +    uint64_t table_size; /* unit is clusters */
>> +} QBlockFmtOptionQed;
>
> Similarly use uppercase for QED, QCOW, etc.
>
>> +typedef struct QBlockFmtOptionQcow {
>> +    uint64_t virt_size;
>> +    QBlockProtInfo backing_loc;
>> +    bool encrypt;
>> +} QBlockFmtOptionQcow;
>> +
>> +/* "Compatibility level (0.10 or 1.1)" */
>> +typedef enum QBlockFmtOptionQcow2CptLv {
>> +    QBO_FMT_QCOW2_CPT_NONE = 0,
>> +    QBO_FMT_QCOW2_CPT_V010,
>> +    QBO_FMT_QCOW2_CPT_V110,
>> +} QBlockFmtOptionQcow2CptLv;
>
> Please use QBO_ instead of QB_ throughout.  Also write COMPAT instead of
> CPT, and remove CPT_NONE since 0.10 is the default:
>
   __NONE is used to indicate whether this property is set or get, so
it is actually have 3 status than just 2: Not set, V010, V110. It
is the same reason that I use __NONE in bool values, especially __NONE 
could represent it is not got in information retrieving. Maybe I should
rename them to __NOTSET.

> typedef enum QBlockFmtOptionQCOW2Compat {
>      QB_FMT_QCOW2_COMPAT_V0_10,
>      QB_FMT_QCOW2_COMPAT_V1_1,
> } QBlockFmtOptionQCOW2Compat
>
>> +/* off or metadata */
>> +typedef enum QBlockFmtOptionQcow2PreAllocType {
>> +    QBO_FMT_QCOW2_PREALLOC_NONE = 0,
>> +    QBO_FMT_QCOW2_PREALLOC_OFF,
>> +    QBO_FMT_QCOW2_PREALLOC_METADATA,
>> +} QBlockFmtOptionQcow2PreAllocType;
>
> Please remove NONE since OFF is the default.  However, I would use the enum.
>
>> +typedef struct QBlockFmtOptionQcow2 {
>> +    uint64_t virt_size;
>> +    QBlockProtInfo backing_loc;
>> +    QBlockFmtType backing_fmt;
>> +    bool encrypt;
>> +    uint64_t cluster_size; /* unit is bytes */
>> +    QBlockFmtOptionQcow2CptLv cpt_lv;
>> +    QBlockFmtOptionQcow2PreAllocType pre_mode;
>> +} QBlockFmtOptionQcow2;
>> +
>> +typedef struct QBlockFmtOptionRaw {
>> +    uint64_t virt_size;
>> +} QBlockFmtOptionRaw;
>> +
>> +typedef struct QBlockFmtOptionRbd {
>> +    uint64_t virt_size;
>> +    uint64_t cluster_size;
>> +} QBlockFmtOptionRbd;
>> +
>> +/* off or full */
>> +typedef enum QBlockFmtOptionSheepdogPreAllocType {
>> +    QBO_FMT_SD_PREALLOC_NONE = 0,
>> +    QBO_FMT_SD_PREALLOC_OFF,
>> +    QBO_FMT_SD_PREALLOC_FULL,
>> +} QBlockFmtOptionSheepdogPreAllocType;
>
> The default is false, so just make it a bool.
>
>> +typedef struct QBlockFmtOptionSheepdog {
>> +    uint64_t virt_size;
>> +    QBlockProtInfo backing_loc;
>> +    QBlockFmtOptionSheepdogPreAllocType pre_mode;
>> +} QBlockFmtOptionSheepdog;
>> +
>> +typedef enum QBlockFmtOptionVdiPreAllocType {
>> +    QBO_FMT_VDI_PREALLOC_NONE = 0,
>> +    QBO_FMT_VDI_PREALLOC_FALSE,
>> +    QBO_FMT_VDI_PREALLOC_TRUE,
>> +} QBlockFmtOptionVdiPreAllocType;
>
> Please remove NONE, and replace FALSE/TRUE with OFF/METADATA (same as
> qcow2).
>
>> +typedef struct QBlockFmtOptionVdi {
>> +    uint64_t virt_size;
>> +    uint64_t cluster_size;
>> +    QBlockFmtOptionVdiPreAllocType pre_mode;
>> +} QBlockFmtOptionVdi;
>> +
>> +/* whether compact to vmdk verion 6 */
>> +typedef enum QBlockFmtOptionVmdkCptLv {
>> +    QBO_FMT_VMDK_CPT_NONE = 0,
>> +    QBO_FMT_VMDK_CPT_VMDKV6_FALSE,
>> +    QBO_FMT_VMDK_CPT_VMDKV6_TRUE,
>> +} QBlockFmtOptionVmdkCptLv;
>
> Here the default is false, so just make it a bool.
>
>> +/* vmdk flat extent format, values:
>> +"{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse |
>> +twoGbMaxExtentFlat | streamOptimized} */
>> +typedef enum QBlockFmtOptionVmdkSubfmtType {
>> +    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE = 0,
>> +    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE,
>
> You can remove QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE (whose more
> appropriate name would be just ...SUBFMT_NONE) since the default is
> QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE.
>
>> +    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_FLAT,
>> +    QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_SPARSE,
>> +    QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_FLAT,
>> +    QBO_FMT_VMDK_SUBFMT_STREAM_OPTIMIZED,
>> +} QBlockFmtOptionVmdkSubfmtType;
>> +
>> +typedef struct QBlockFmtOptionVmdk {
>> +    uint64_t virt_size;
>> +    QBlockProtInfo backing_loc;
>> +    QBlockFmtOptionVmdkCptLv cpt_lv;
>> +    QBlockFmtOptionVmdkSubfmtType subfmt;
>> +} QBlockFmtOptionVmdk;
>> +
>> +/* "{dynamic (default) | fixed} " */
>> +typedef enum QBlockFmtOptionVpcSubfmtType {
>> +    QBO_FMT_VPC_SUBFMT_NONE = 0,
>> +    QBO_FMT_VPC_SUBFMT_DYNAMIC,
>> +    QBO_FMT_VPC_SUBFMT_FIXED,
>> +} QBlockFmtOptionVpcSubfmtType;
>
> Again, please remove NONE.
>
>> +typedef struct QBlockFmtOptionVpc {
>> +    uint64_t virt_size;
>> +    QBlockFmtOptionVpcSubfmtType subfmt;
>> +} QBlockFmtOptionVpc;
>> +
>> +#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2)
>> +typedef union QBlockFmtOptionsUnion {
>> +    QBlockFmtOptionCow       o_cow;
>> +    QBlockFmtOptionQed       o_qed;
>> +    QBlockFmtOptionQcow      o_qcow;
>> +    QBlockFmtOptionQcow2     o_qcow2;
>> +    QBlockFmtOptionRaw       o_raw;
>> +    QBlockFmtOptionRbd       o_rbd;
>> +    QBlockFmtOptionSheepdog  o_sheepdog;
>> +    QBlockFmtOptionVdi       o_vdi;
>> +    QBlockFmtOptionVmdk      o_vmdk;
>> +    QBlockFmtOptionVpc       o_vpc;
>> +    uint8_t reserved[QBLOCK_FMT_OPTIONS_UNION_SIZE];
>
> Similarly use uint64_t here, and just embed the union in the struct.
>
>> +} QBlockFmtOptionsUnion;
>> +typedef struct QBlockFmtInfo {
>> +    QBlockFmtType fmt_type;
>> +    QBlockFmtOptionsUnion fmt_op;
>> +} QBlockFmtInfo;
>
> QBlockFormatInfo.
>
>> +/**
>> + * QBlockStaticInfoAddr: a structure contains a set of pointer.
>> + *
>> + *    this struct contains a set of pointer pointing to some
>> + *  property related to format or protocol. If a property is not available,
>> + *  it will be set as NULL. User could use this to get properties directly.
>> + *
>> + *  @virt_size: virtual size, it is always not NULL.
>> + *  @backing_loc: backing file location.
>> + *  @encrypt: encryption flag.
>> +*/
>> +
>> +typedef struct QBlockStaticInfoAddr {
>> +    uint64_t *virt_size;
>> +    QBlockProtInfo *backing_loc;
>> +    bool *encrypt;
>> +} QBlockStaticInfoAddr;
>
> Why the indirection?
>
   It helps user to get these important members, otherwise
user will need
Switch (fmt) {
   case RAW:
     ....
   case QCOW2:
     ...
}
for every attribute. The indirection address will let user directly
access the members.

>> +/**
>> + * QBlockStaticInfo: information about the block image.
>> + *
>> + * @member_addr: contains pointer which can be used to access the structure.
>> + * @loc: location information.
>> + * @fmt: format information.
>> + * @sector_size: how many bytes in a sector, it is 512 usually.
>> + */
>> +typedef struct QBlockStaticInfo {
>> +    QBlockStaticInfoAddr *member_addr;
>> +    QBlockProtInfo loc;
>> +    QBlockFmtInfo fmt;
>> +    int sector_size;
>> +} QBlockStaticInfo;
>> +
>> +
>> +#endif
>>
>


-- 
Best Regards

Wenchao Xia