[V2,2/6] libqblock type and structure defines

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

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?

于 2012-9-11 5:27, Eric Blake 写道:
> 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.
>
   OK.

>> 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.
>
   Yes it will, thanks for mention me. The resource will always have a
limit, I guess I can just increase the size to 4k to solve the issue.

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

>> +
>> +/**
>> + * 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?
>
   It could have or not. If new members are added in the tail, then
reserved bytes is not needed, for that this structure's instance
will be allocated in libqblock API.

On 09/10/2012 09:26 PM, Wenchao Xia wrote:

>>> +#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.
>>
>   Yes it will, thanks for mention me. The resource will always have a
> limit, I guess I can just increase the size to 4k to solve the issue.

A 4k limit is still an easily reachable limit.  PATH_MAX is typically
4k, and it is quite possible to create and access files in a hierarchy
so deep that they are longer than PATH_MAX.  I still think you are
better off malloc'ing a pointer than trying to claim a fixed width field
solves all possible messages.

On Mon, Sep 10, 2012 at 8:26 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> 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
>
> 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 @@
> +/*
> + * 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 "block.h"
> +#include "block_int.h"
> +
> +#include "libqblock-types.h"
> +
> +/* this file contains defines and types used inside the library. */
> +
> +#define FUNC_FREE free
> +#define FUNC_MALLOC malloc
> +#define FUNC_CALLOC calloc
> +
> +#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;
> +};
> +
> +#define QB_ERR_STRING_SIZE (1024)
> +struct QBroker {
> +    /* last error */
> +    char err_msg[QB_ERR_STRING_SIZE];
> +    int err_ret; /* last error return of libqblock. */
> +    int err_no; /* 2nd level of error, errno what below reports */
> +};
> +
> +#endif
> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h
> new file mode 100644
> index 0000000..9d4e3fc
> --- /dev/null
> +++ b/libqblock/libqblock-types.h
> @@ -0,0 +1,251 @@
> +/*
> + * 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 __GNUC__ >= 4

#if defined(__GNUC__) && __GNUC__ ...

> +    #ifdef LIBQB_BUILD
> +        #define DLL_PUBLIC __attribute__((visibility("default")))
> +    #else
> +        #define DLL_PUBLIC
> +    #endif
> +#endif
> +
> +/* this library is designed around this core struct. */
> +struct QBlockState;

typedefs missing

> +
> +/* every thread would have a broker. */
> +struct QBroker;
> +
> +/* 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)
> +
> +enum QBlockProtType {
> +    QB_PROT_NONE = 0,
> +    QB_PROT_FILE,
> +    QB_PROT_MAX
> +};
> +
> +struct QBlockProtOptionFile {
> +    const char *filename;
> +};
> +
> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512)
> +union QBlockProtOptionsUnion {
> +    struct QBlockProtOptionFile o_file;
> +    uint8_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE];
> +};
> +
> +/**
> + * struct QBlockProtInfo: contains information about how to find the image
> + *
> + * @prot_type: protocol type, now only support FILE.
> + * @prot_op: protocol related options.
> + */
> +struct QBlockProtInfo {
> +    enum QBlockProtType prot_type;
> +    union QBlockProtOptionsUnion prot_op;
> +};
> +
> +
> +/* format related options */
> +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
> +};
> +
> +struct QBlockFmtOptionCow {
> +    uint64_t virt_size;
> +    struct QBlockProtInfo backing_loc;
> +};
> +
> +struct QBlockFmtOptionQed {
> +    uint64_t virt_size;
> +    struct QBlockProtInfo backing_loc;
> +    enum QBlockFmtType backing_fmt;
> +    uint64_t cluster_size; /* unit is bytes */
> +    uint64_t table_size; /* unit is clusters */
> +};
> +
> +struct QBlockFmtOptionQcow {
> +    uint64_t virt_size;
> +    struct QBlockProtInfo backing_loc;
> +    bool encrypt;
> +};
> +
> +/* "Compatibility level (0.10 or 1.1)" */
> +enum QBlockFmtOptionQcow2CptLv {
> +    QBO_FMT_QCOW2_CPT_NONE = 0,
> +    QBO_FMT_QCOW2_CPT_V010,
> +    QBO_FMT_QCOW2_CPT_V110,
> +};
> +
> +/* off or metadata */
> +enum QBlockFmtOptionQcow2PreAllocType {
> +    QBO_FMT_QCOW2_PREALLOC_NONE = 0,
> +    QBO_FMT_QCOW2_PREALLOC_OFF,
> +    QBO_FMT_QCOW2_PREALLOC_METADATA,
> +};
> +
> +struct QBlockFmtOptionQcow2 {
> +    uint64_t virt_size;
> +    struct QBlockProtInfo backing_loc;
> +    enum QBlockFmtType backing_fmt;
> +    bool encrypt;
> +    uint64_t cluster_size; /* unit is bytes */
> +    enum QBlockFmtOptionQcow2CptLv cpt_lv;
> +    enum QBlockFmtOptionQcow2PreAllocType pre_mode;
> +};
> +
> +struct QBlockFmtOptionRaw {
> +    uint64_t virt_size;
> +};
> +
> +struct QBlockFmtOptionRbd {
> +    uint64_t virt_size;
> +    uint64_t cluster_size;
> +};
> +
> +/* off or full */
> +enum QBlockFmtOptionSheepdogPreAllocType {
> +    QBO_FMT_SD_PREALLOC_NONE = 0,
> +    QBO_FMT_SD_PREALLOC_OFF,
> +    QBO_FMT_SD_PREALLOC_FULL,
> +};
> +
> +struct QBlockFmtOptionSheepdog {
> +    uint64_t virt_size;
> +    struct QBlockProtInfo backing_loc;
> +    enum QBlockFmtOptionSheepdogPreAllocType pre_mode;
> +};
> +
> +enum QBlockFmtOptionVdiPreAllocType {
> +    QBO_FMT_VDI_PREALLOC_NONE = 0,
> +    QBO_FMT_VDI_PREALLOC_FALSE,
> +    QBO_FMT_VDI_PREALLOC_TRUE,
> +};
> +
> +struct QBlockFmtOptionVdi {
> +    uint64_t virt_size;
> +    uint64_t cluster_size;
> +    enum QBlockFmtOptionVdiPreAllocType pre_mode;
> +};
> +
> +/* whether compact to vmdk verion 6 */
> +enum QBlockFmtOptionVmdkCptLv {
> +    QBO_FMT_VMDK_CPT_NONE = 0,
> +    QBO_FMT_VMDK_CPT_VMDKV6_FALSE,
> +    QBO_FMT_VMDK_CPT_VMDKV6_TRUE,
> +};
> +
> +/* vmdk flat extent format, values:
> +"{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse |
> +twoGbMaxExtentFlat | streamOptimized} */
> +enum QBlockFmtOptionVmdkSubfmtType {
> +    QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE = 0,
> +    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,
> +};
> +
> +struct QBlockFmtOptionVmdk {
> +    uint64_t virt_size;
> +    struct QBlockProtInfo backing_loc;
> +    enum QBlockFmtOptionVmdkCptLv cpt_lv;
> +    enum QBlockFmtOptionVmdkSubfmtType subfmt;
> +};
> +
> +/* "{dynamic (default) | fixed} " */
> +enum QBlockFmtOptionVpcSubfmtType {
> +    QBO_FMT_VPC_SUBFMT_NONE = 0,
> +    QBO_FMT_VPC_SUBFMT_DYNAMIC,
> +    QBO_FMT_VPC_SUBFMT_FIXED,
> +};
> +
> +struct QBlockFmtOptionVpc {
> +    uint64_t virt_size;
> +    enum QBlockFmtOptionVpcSubfmtType subfmt;
> +};
> +
> +#define QBLOCK_FMT_OPTIONS_UNION_SIZE (QBLOCK_PROT_OPTIONS_UNION_SIZE*2)
> +union QBlockFmtOptionsUnion {
> +    struct QBlockFmtOptionCow       o_cow;
> +    struct QBlockFmtOptionQed       o_qed;
> +    struct QBlockFmtOptionQcow      o_qcow;
> +    struct QBlockFmtOptionQcow2     o_qcow2;
> +    struct QBlockFmtOptionRaw       o_raw;
> +    struct QBlockFmtOptionRbd       o_rbd;
> +    struct QBlockFmtOptionSheepdog  o_sheepdog;
> +    struct QBlockFmtOptionVdi       o_vdi;
> +    struct QBlockFmtOptionVmdk      o_vmdk;
> +    struct QBlockFmtOptionVpc       o_vpc;
> +    uint8_t reserved[QBLOCK_FMT_OPTIONS_UNION_SIZE];
> +};
> +
> +struct QBlockFmtInfo {
> +    enum QBlockFmtType fmt_type;
> +    union QBlockFmtOptionsUnion fmt_op;
> +};
> +
> +/**
> + * 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;
> +};
> +#endif
> --
> 1.7.1
>
>

On 09/11/2012 02:31 PM, Blue Swirl wrote:
> On Mon, Sep 10, 2012 at 8:26 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>   This patch contains type and defines used in APIs, one file for public usage
>> by user, one for libqblock internal usage.
>>

>> +
>> +#if __GNUC__ >= 4
> 
> #if defined(__GNUC__) && __GNUC__ ...

Seriously?  We require a C99-compliant compiler, which is required to
treat the more compact version identically (all undefined names evaluate
to 0 in the preprocessor), and HACKING doesn't mandate that we spell out
a defined-ness check first.  Okay, so configure adds -Wundef to the set
of CFLAGS, but I fail to see why we want -Wundef (that's an
anachronistic warning, only there to help you if you are writing code
portable to K&R).

于 2012-9-12 6:52, Eric Blake 写道:
> On 09/11/2012 02:31 PM, Blue Swirl wrote:
>> On Mon, Sep 10, 2012 at 8:26 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>    This patch contains type and defines used in APIs, one file for public usage
>>> by user, one for libqblock internal usage.
>>>
>
>>> +
>>> +#if __GNUC__ >= 4
>>
>> #if defined(__GNUC__) && __GNUC__ ...
>
> Seriously?  We require a C99-compliant compiler, which is required to
> treat the more compact version identically (all undefined names evaluate
> to 0 in the preprocessor), and HACKING doesn't mandate that we spell out
> a defined-ness check first.  Okay, so configure adds -Wundef to the set
> of CFLAGS, but I fail to see why we want -Wundef (that's an
> anachronistic warning, only there to help you if you are writing code
> portable to K&R).
>
   So if the preprocessor replaced __GNUC__ to 0, is there difference
between these two kinds of macoros?
#if __GNUC__ >= 4
#if defined(__GNUC__) && __GNUC__ >=4

   I guess they are the same.

On 09/11/2012 09:05 PM, Wenchao Xia wrote:
>> Seriously?  We require a C99-compliant compiler, which is required to
>> treat the more compact version identically (all undefined names evaluate
>> to 0 in the preprocessor), and HACKING doesn't mandate that we spell out
>> a defined-ness check first.  Okay, so configure adds -Wundef to the set
>> of CFLAGS, but I fail to see why we want -Wundef (that's an
>> anachronistic warning, only there to help you if you are writing code
>> portable to K&R).
>>
>   So if the preprocessor replaced __GNUC__ to 0, is there difference
> between these two kinds of macoros?
> #if __GNUC__ >= 4
> #if defined(__GNUC__) && __GNUC__ >=4

The only difference is whether -Wundef will warn, and I'm trying to
argue that qemu's current use of -Wundef is pointless, as that warning
exists solely for K&R compatibility, not for modern standard-compliant
code correctness.

于 2012-9-12 20:59, Eric Blake 写道:
> On 09/11/2012 09:05 PM, Wenchao Xia wrote:
>>> Seriously?  We require a C99-compliant compiler, which is required to
>>> treat the more compact version identically (all undefined names evaluate
>>> to 0 in the preprocessor), and HACKING doesn't mandate that we spell out
>>> a defined-ness check first.  Okay, so configure adds -Wundef to the set
>>> of CFLAGS, but I fail to see why we want -Wundef (that's an
>>> anachronistic warning, only there to help you if you are writing code
>>> portable to K&R).
>>>
>>    So if the preprocessor replaced __GNUC__ to 0, is there difference
>> between these two kinds of macoros?
>> #if __GNUC__ >= 4
>> #if defined(__GNUC__) && __GNUC__ >=4
>
> The only difference is whether -Wundef will warn, and I'm trying to
> argue that qemu's current use of -Wundef is pointless, as that warning
> exists solely for K&R compatibility, not for modern standard-compliant
> code correctness.
>
  OK ,then I think
#if __GNUC__ >= 4
....
#else
   [warn name space pollution may happen]
#endif
would be better.

On 09/12/2012 09:24 PM, Wenchao Xia wrote:
> 于 2012-9-12 20:59, Eric Blake 写道:
>> On 09/11/2012 09:05 PM, Wenchao Xia wrote:
>>>> Seriously?  We require a C99-compliant compiler, which is required to
>>>> treat the more compact version identically (all undefined names
>>>> evaluate
>>>> to 0 in the preprocessor), and HACKING doesn't mandate that we spell
>>>> out
>>>> a defined-ness check first.  Okay, so configure adds -Wundef to the set
>>>> of CFLAGS, but I fail to see why we want -Wundef (that's an
>>>> anachronistic warning, only there to help you if you are writing code
>>>> portable to K&R).
>>>>
>>>    So if the preprocessor replaced __GNUC__ to 0, is there difference
>>> between these two kinds of macoros?
>>> #if __GNUC__ >= 4
>>> #if defined(__GNUC__) && __GNUC__ >=4
>>
>> The only difference is whether -Wundef will warn, and I'm trying to
>> argue that qemu's current use of -Wundef is pointless, as that warning
>> exists solely for K&R compatibility, not for modern standard-compliant
>> code correctness.
>>
>  OK ,then I think
> #if __GNUC__ >= 4
> ....
> #else
>   [warn name space pollution may happen]
> #endif
> would be better.

It may be shorter, but it is definitely not better, at least not in the
current context of qemu.  Using the short form will fail a -Werror
build, unless you also write a patch to qemu's configure to quit
supplying -Wundef during builds.  But as touching configure has a bigger
impact to the overall qemu project, you're going to need a lot more
buy-in from other developers that -Wundef is not helping qemu gain any
portability, and that it is safe to ditch it (or get enough
counter-arguments from other developers why qemu insists on the
anachronistic style enforced by -Wundef, at which point you must comply
and use the longer form).

On 09/12/2012 09:33 PM, Eric Blake wrote:
>>  OK ,then I think
>> #if __GNUC__ >= 4
>> ....
>> #else
>>   [warn name space pollution may happen]
>> #endif
>> would be better.
> 
> It may be shorter, but it is definitely not better, at least not in the
> current context of qemu.  Using the short form will fail a -Werror
> build, unless you also write a patch to qemu's configure to quit
> supplying -Wundef during builds.  But as touching configure has a bigger
> impact to the overall qemu project, you're going to need a lot more
> buy-in from other developers that -Wundef is not helping qemu gain any
> portability, and that it is safe to ditch it (or get enough
> counter-arguments from other developers why qemu insists on the
> anachronistic style enforced by -Wundef, at which point you must comply
> and use the longer form).

On second thought, this _particular_ usage will never fail a -Wundef
-Werror build, precisely because -Wundef is a gcc warning, which impies
the warning is only ever useful in the same scenarios that the __GNUC__
macro is always defined (that is, __GNUC__ is undefined only on a
non-gcc compiler, but what non-gcc compiler supports -Wundef -Werror?).

But why should this line be the one exemption to the rules?  Either qemu
insists on the -Wundef style of coding (and you should use the long form
to conform to that style, on the off-chance that someone ever wants to
port to a non-gcc compiler, even in this one place where gcc can't warn
you about the violation of that style), or we should change the qemu
style (at which point, the short form is nicer here, but it also implies
the potential for cleaning up lots of other places to also use short
forms and rely on preprocessor 0 computation).

On Tue, Sep 11, 2012 at 10:52 PM, Eric Blake <eblake@redhat.com> wrote:
> On 09/11/2012 02:31 PM, Blue Swirl wrote:
>> On Mon, Sep 10, 2012 at 8:26 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>   This patch contains type and defines used in APIs, one file for public usage
>>> by user, one for libqblock internal usage.
>>>
>
>>> +
>>> +#if __GNUC__ >= 4
>>
>> #if defined(__GNUC__) && __GNUC__ ...
>
> Seriously?  We require a C99-compliant compiler, which is required to
> treat the more compact version identically (all undefined names evaluate
> to 0 in the preprocessor), and HACKING doesn't mandate that we spell out
> a defined-ness check first.  Okay, so configure adds -Wundef to the set
> of CFLAGS, but I fail to see why we want -Wundef (that's an
> anachronistic warning, only there to help you if you are writing code
> portable to K&R).

I don't think we require C99 compliance. Removing -Wundef may have
merits of its own even if I fail to see them. Typically a stricter set
of warnings is better.

But this case is different from other QEMU since this is supposed to
be a public header file for a shared library that will be used by
projects external to QEMU. We have absolutely no control what compiler
flags the user project is using, so we should try to be compatible
with any reasonable compilers with any reasonable settings. We could
even try to be compatible with C++.

The above applies only to these headers. Aiming for compatibility for
other compilers for the .c files would make sense if there was a lot
of interest to use only the library directly in another project, but
let's wait for the interested crowds to show up.

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

On Thu, Sep 13, 2012 at 3:49 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/12/2012 09:33 PM, Eric Blake wrote:
>>>  OK ,then I think
>>> #if __GNUC__ >= 4
>>> ....
>>> #else
>>>   [warn name space pollution may happen]
>>> #endif
>>> would be better.
>>
>> It may be shorter, but it is definitely not better, at least not in the
>> current context of qemu.  Using the short form will fail a -Werror
>> build, unless you also write a patch to qemu's configure to quit
>> supplying -Wundef during builds.  But as touching configure has a bigger
>> impact to the overall qemu project, you're going to need a lot more
>> buy-in from other developers that -Wundef is not helping qemu gain any
>> portability, and that it is safe to ditch it (or get enough
>> counter-arguments from other developers why qemu insists on the
>> anachronistic style enforced by -Wundef, at which point you must comply
>> and use the longer form).
>
> On second thought, this _particular_ usage will never fail a -Wundef
> -Werror build, precisely because -Wundef is a gcc warning, which impies
> the warning is only ever useful in the same scenarios that the __GNUC__
> macro is always defined (that is, __GNUC__ is undefined only on a
> non-gcc compiler, but what non-gcc compiler supports -Wundef -Werror?).

The library could be used by a project that does not use GCC or pick
CFLAGS from QEMU configuration. Supporting for example MSVC or C++
users for the library could be interesting one day, even if we didn't
support MSVC or C++ at all for building the rest of QEMU.

>
> But why should this line be the one exemption to the rules?  Either qemu
> insists on the -Wundef style of coding (and you should use the long form
> to conform to that style, on the off-chance that someone ever wants to
> port to a non-gcc compiler, even in this one place where gcc can't warn
> you about the violation of that style), or we should change the qemu
> style (at which point, the short form is nicer here, but it also implies
> the potential for cleaning up lots of other places to also use short
> forms and rely on preprocessor 0 computation).
>
> --
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

于 2012-9-15 2:11, Blue Swirl 写道:
> On Thu, Sep 13, 2012 at 3:49 AM, Eric Blake <eblake@redhat.com> wrote:
>> On 09/12/2012 09:33 PM, Eric Blake wrote:
>>>>   OK ,then I think
>>>> #if __GNUC__ >= 4
>>>> ....
>>>> #else
>>>>    [warn name space pollution may happen]
>>>> #endif
>>>> would be better.
>>>
>>> It may be shorter, but it is definitely not better, at least not in the
>>> current context of qemu.  Using the short form will fail a -Werror
>>> build, unless you also write a patch to qemu's configure to quit
>>> supplying -Wundef during builds.  But as touching configure has a bigger
>>> impact to the overall qemu project, you're going to need a lot more
>>> buy-in from other developers that -Wundef is not helping qemu gain any
>>> portability, and that it is safe to ditch it (or get enough
>>> counter-arguments from other developers why qemu insists on the
>>> anachronistic style enforced by -Wundef, at which point you must comply
>>> and use the longer form).
>>
>> On second thought, this _particular_ usage will never fail a -Wundef
>> -Werror build, precisely because -Wundef is a gcc warning, which impies
>> the warning is only ever useful in the same scenarios that the __GNUC__
>> macro is always defined (that is, __GNUC__ is undefined only on a
>> non-gcc compiler, but what non-gcc compiler supports -Wundef -Werror?).
>
> The library could be used by a project that does not use GCC or pick
> CFLAGS from QEMU configuration. Supporting for example MSVC or C++
> users for the library could be interesting one day, even if we didn't
> support MSVC or C++ at all for building the rest of QEMU.
>
   Each compiler would have its own predefined macro, so I think now
I can just support gcc and give a warning when gcc not found. If
more compiler is needed, extend the macro in the future.

>>
>> But why should this line be the one exemption to the rules?  Either qemu
>> insists on the -Wundef style of coding (and you should use the long form
>> to conform to that style, on the off-chance that someone ever wants to
>> port to a non-gcc compiler, even in this one place where gcc can't warn
>> you about the violation of that style), or we should change the qemu
>> style (at which point, the short form is nicer here, but it also implies
>> the potential for cleaning up lots of other places to also use short
>> forms and rely on preprocessor 0 computation).
>>
>> --
>> Eric Blake   eblake@redhat.com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>>
>

On Mon, Sep 17, 2012 at 2:23 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2012-9-15 2:11, Blue Swirl 写道:
>
>> On Thu, Sep 13, 2012 at 3:49 AM, Eric Blake <eblake@redhat.com> wrote:
>>>
>>> On 09/12/2012 09:33 PM, Eric Blake wrote:
>>>>>
>>>>>   OK ,then I think
>>>>> #if __GNUC__ >= 4
>>>>> ....
>>>>> #else
>>>>>    [warn name space pollution may happen]
>>>>> #endif
>>>>> would be better.
>>>>
>>>>
>>>> It may be shorter, but it is definitely not better, at least not in the
>>>> current context of qemu.  Using the short form will fail a -Werror
>>>> build, unless you also write a patch to qemu's configure to quit
>>>> supplying -Wundef during builds.  But as touching configure has a bigger
>>>> impact to the overall qemu project, you're going to need a lot more
>>>> buy-in from other developers that -Wundef is not helping qemu gain any
>>>> portability, and that it is safe to ditch it (or get enough
>>>> counter-arguments from other developers why qemu insists on the
>>>> anachronistic style enforced by -Wundef, at which point you must comply
>>>> and use the longer form).
>>>
>>>
>>> On second thought, this _particular_ usage will never fail a -Wundef
>>> -Werror build, precisely because -Wundef is a gcc warning, which impies
>>> the warning is only ever useful in the same scenarios that the __GNUC__
>>> macro is always defined (that is, __GNUC__ is undefined only on a
>>> non-gcc compiler, but what non-gcc compiler supports -Wundef -Werror?).
>>
>>
>> The library could be used by a project that does not use GCC or pick
>> CFLAGS from QEMU configuration. Supporting for example MSVC or C++
>> users for the library could be interesting one day, even if we didn't
>> support MSVC or C++ at all for building the rest of QEMU.
>>
>   Each compiler would have its own predefined macro, so I think now
> I can just support gcc and give a warning when gcc not found. If
> more compiler is needed, extend the macro in the future.

Yes, that's OK for now. It's also possible that any interest for
non-GCC compilation or C++ will never materialize. But please support
-Wundef.

>
>
>>>
>>> But why should this line be the one exemption to the rules?  Either qemu
>>> insists on the -Wundef style of coding (and you should use the long form
>>> to conform to that style, on the off-chance that someone ever wants to
>>> port to a non-gcc compiler, even in this one place where gcc can't warn
>>> you about the violation of that style), or we should change the qemu
>>> style (at which point, the short form is nicer here, but it also implies
>>> the potential for cleaning up lots of other places to also use short
>>> forms and rely on preprocessor 0 computation).
>>>
>>> --
>>> Eric Blake   eblake@redhat.com    +1-919-301-3266
>>> Libvirt virtualization library http://libvirt.org
>>>
>>
>
>
> --
> Best Regards
>
> Wenchao Xia
>