[V17,07/10] libqblock: libqblock API design and type defines

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 QBlockContext. This structure was used to retrieve errors, every thread
must create one first.
  2 QBlockImage. It stands for an block image object.
  3 QBlockStaticInfo. It contains static information such as location, backing
file, size.
  4 Sync I/O. It is similar to C file open, read, write and close operations.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 libqblock/libqblock-error.h    |   50 ++++++
 libqblock/libqblock-internal.h |   68 ++++++++
 libqblock/libqblock-types.h    |  245 +++++++++++++++++++++++++++
 libqblock/libqblock.h          |  358 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 721 insertions(+), 0 deletions(-)
 create mode 100644 libqblock/libqblock-internal.h

On Thu, Jan 31, 2013 at 04:53:47PM +0800, 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 QBlockContext. This structure was used to retrieve errors, every thread
> must create one first.
>   2 QBlockImage. It stands for an block image object.
>   3 QBlockStaticInfo. It contains static information such as location, backing
> file, size.
>   4 Sync I/O. It is similar to C file open, read, write and close operations.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  libqblock/libqblock-error.h    |   50 ++++++
>  libqblock/libqblock-internal.h |   68 ++++++++
>  libqblock/libqblock-types.h    |  245 +++++++++++++++++++++++++++
>  libqblock/libqblock.h          |  358 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 721 insertions(+), 0 deletions(-)
>  create mode 100644 libqblock/libqblock-internal.h
> 
> diff --git a/libqblock/libqblock-error.h b/libqblock/libqblock-error.h
> index e69de29..1907ecf 100644
> --- a/libqblock/libqblock-error.h
> +++ b/libqblock/libqblock-error.h
> @@ -0,0 +1,50 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * 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_ERROR
> +#define LIBQBLOCK_ERROR
> +
> +#include "libqblock-types.h"
> +
> +#define QB_ERR_INTERNAL_ERR (-1)
> +#define QB_ERR_FATAL_ERR (-2)
> +#define QB_ERR_INVALID_PARAM (-100)
> +#define QB_ERR_BLOCK_OUT_OF_RANGE (-101)
> +
> +/* error handling */
> +/**
> + * qb_error_get_human_str: get human readable error string.
> + *
> + * return a human readable string, it would be truncated if buf is not big
> + *  enough.
> + *
> + * @context: operation context, must be valid.
> + * @buf: buf to receive the string.
> + * @buf_size: the size of the string buf.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_error_get_human_str(QBlockContext *context,
> +                            char *buf, size_t buf_size);

This function is broken.  There is no way to find out how big buf must
be and there is no way to find if the result is truncated or not.

This is actually worse than:
#define QB_ERROR_HUMAN_SIZE 512
/* buf must be QB_ERROR_HUMAN_SIZE */
void qb_error_get_human_str(QBlockContext *context, char *buf);

At least here the caller knows they will get the full error message
because the library designer knows the error message lengths better than
the application writer.

The nicest would be:

/* Return human-readable error message.  The caller must use free(3). */
char *qb_error_get_human_str(QBlockContext *context);

> +
> +/**
> + * qb_error_get_errno: get error number, only valid when err_ret is
> + *   QB_ERR_INTERNAL_ERR.
> + *
> + * return negative errno if last error is QB_ERR_INTERNAL_ERR, otherwise 0.
> + *
> + * @context: operation context.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_error_get_errno(QBlockContext *context);

QB_ERR_INTERNAL_ERR is an internal error which means the caller doesn't
know exactly what happened.  How is the application supposed to use the
errno?

The only thing the application could do is strerror(3) but
qb_error_get_human_str() should already do that.

Therefore qb_error_get_errno() serves no purpose.

> +
> +#endif
> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h
> new file mode 100644
> index 0000000..91521f5
> --- /dev/null
> +++ b/libqblock/libqblock-internal.h
> @@ -0,0 +1,68 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * 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/block.h"
> +#include "libqblock-types.h"
> +
> +/* this file contains defines and types used inside the library. */
> +
> +#define QB_FREE(p) { \
> +        g_free(p); \
> +        (p) = NULL; \
> +}
> +
> +/* details should be hidden to user */
> +struct QBlockImage {
> +    BlockDriverState *bdrvs;
> +    /* internal used file name now, if it is not NULL, it means
> +       image was opened.
> +    */
> +    char *filename;
> +    int ref_count;
> +};
> +
> +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 */
> +};
> +
> +/**
> + * 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.
> + *
> + *  @backing_loc: backing file location.
> + *  @encrypt: encryption flag.
> +*/
> +
> +typedef struct QBlockStaticInfoAddr {
> +    QBlockLocationInfo *backing_loc;
> +    bool *encrypt;
> +} QBlockStaticInfoAddr;
> +
> +#define G_LIBQBLOCK_ERROR g_libqblock_error_quark()
> +
> +static inline GQuark g_libqblock_error_quark(void)
> +{
> +    return g_quark_from_static_string("g-libqblock-error-quark");
> +}

qb_error_quark() would be a better name.  g_*() is for GLib not for code
using glib.

The shorter name also means you can drop the macro.  Macros like
QB_FREE() and G_LIBQBLOCK_ERROR hide a very simple operation.  They
force people reading the code to jump to its definition; it interrupts
the reader and forces them to remember abstractions that add no value.

> +#endif
> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h
> index e69de29..d49b321 100644
> --- a/libqblock/libqblock-types.h
> +++ b/libqblock/libqblock-types.h
> @@ -0,0 +1,245 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * 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>
> +
> +/*
> + * In case libqblock is used by other project which have not defined
> + * QEMU_DLL_PUBLIC.
> + */
> +#ifndef QEMU_DLL_PUBLIC
> +#define QEMU_DLL_PUBLIC
> +#endif
> +
> +/* this library is designed around this core struct. */
> +typedef struct QBlockImage QBlockImage;
> +
> +/* 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)

It is tempting to simplify the libqblock cache options so that:

 * The host page cache is always used.
 * Flush operations are required to ensure writes reach the disk.

This means disk images would behave like regular files.  And it would
prevent a lot of confusion about the meaning of the cache modes.

However, advanced applications might want to use O_DIRECT or even
NO_FLUSH.  So I guess it's okay to expose this.

> +
> +typedef enum QBlockProtocol {
> +    QB_PROTO_NONE = 0,
> +    QB_PROTO_FILE,
> +    QB_PROTO_MAX
> +} QBlockProtocol;

What prevents libqblock from supporting all protocols?

> +typedef struct QBlockProtocolOptionsFile {
> +    const char *filename;
> +} QBlockProtocolOptionsFile;
> +
> +/**
> + * struct QBlockLocationInfo: contains information about how to find the image
> + *
> + * @prot_type: protocol type, now only support FILE.
> + * @o_file: file protocol related attributes.
> + * @reserved: reserved bytes for ABI.
> + */
> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512)
> +typedef struct QBlockLocationInfo {
> +    QBlockProtocol prot_type;
> +    union {
> +        QBlockProtocolOptionsFile o_file;
> +        uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE/sizeof(uint64_t)];
> +    };
> +} QBlockLocationInfo;
> +
> +
> +/* format related options */
> +typedef enum QBlockFormat {
> +    QB_FORMAT_NONE = 0,
> +    QB_FORMAT_COW,
> +    QB_FORMAT_QED,
> +    QB_FORMAT_QCOW,
> +    QB_FORMAT_QCOW2,
> +    QB_FORMAT_RAW,
> +    QB_FORMAT_RBD,
> +    QB_FORMAT_SHEEPDOG,
> +    QB_FORMAT_VDI,
> +    QB_FORMAT_VMDK,
> +    QB_FORMAT_VPC,
> +    QB_FORMAT_MAX
> +} QBlockFormat;
> +
> +typedef struct QBlockFormatOptionsCOW {
> +    QBlockLocationInfo backing_loc;
> +} QBlockFormatOptionsCOW;

Are these struct definitions frozen?

An application could do:

QBlockFormatInfo info = ...;
QBlockFormatOptionsCOW opts = info.o_cow; /* broken */

If QBlockFormatOptionsCOW changes size then the application can break.
It's hard to demand that applications don't use these structs directly
and once broken applications rely on it we may have a hard time arguing
with them that it's their fault the application is incompatible with new
versions of libqblock (even if we're technically right).

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

...this is an example of a struct where we still need to add fields.
qcow2 is still under development and will get new options.

> diff --git a/libqblock/libqblock.h b/libqblock/libqblock.h
> index e69de29..d3fcaf5 100644
> --- a/libqblock/libqblock.h
> +++ b/libqblock/libqblock.h
> @@ -0,0 +1,358 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * 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.
> + *
> + */
> +
> +/* Note: This library is not thread safe yet. */
> +
> +#ifndef LIBQBLOCK_H
> +#define LIBQBLOCK_H
> +
> +#include "libqblock-types.h"
> +#include "libqblock-error.h"
> +
> +/**
> + * qb_context_new: allocate a new context.
> + *
> + * Broker is used to pass operation to libqblock, and get feedback from it.

Broker?

> + *
> + * Returns 0 on success, libqblock negative error value on fail.
> + *
> + * Note this function will try init the library, so it must be called the
> + * first.
> + *
> + * @p_context: used to receive the created struct.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_context_new(QBlockContext **p_context);
> +
> +/**
> + * qb_context_delete: delete context.
> + *
> + * Broker will be freed and set to NULL.
> + *
> + * @p_context: pointer to the struct pointer, *p_context will be set to NULL.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_context_delete(QBlockContext **p_context);
> +
> +/**
> + * qb_image_new: allocate a new QBlockImage struct.
> + *
> + * Subsequent qblock actions will use this struct, ref is set to 1.
> + *
> + * Returns 0 if succeed, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @p_qbi: used to receive the created struct.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_image_new(QBlockContext *context,
> +                 QBlockImage **p_qbi);
> +
> +/**
> + * qb_image_ref: increase the ref of QBlockImage.
> + *
> + * @context: operation context.
> + * @p_qbi: pointer to the struct's pointer.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_image_ref(QBlockContext *context,
> +                  QBlockImage **p_qbi);

Why QBlockImage** instead of QBlockImage*?

What is the relationship between QBlockImage and its context?  Can I
have two contexts A and B like this:

qb_image_ref(ctx_a, &qbi);
qb_image_unref(ctx_b, &qbi);

Is this okay?

> +
> +/**
> + * qb_image_unref: decrease the ref of QBlockImage.
> + *
> + * if ref is reduced to 0, p_qbi would be freed and set to NULL, at which time
> + *  if image was opened and qb_close was not called before, it would be
> + *  automatically closed.
> + *
> + * @context: operation context.
> + * @p_qbi: pointer to the struct's pointer.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_image_unref(QBlockContext *context,
> +                    QBlockImage **p_qbi);
> +
> +/**
> + * qb_location_info_new: create a new QBlockLocationInfo object.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @p_loc: pointer to receive the new created one.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_location_info_new(QBlockContext *context,
> +                         QBlockLocationInfo **p_loc);
> +
> +/**
> + * qb_location_info_delete: free a QBlockLocationInfo.
> + *
> + * @context: operation context.
> + * @p_loc: pointer to the object, *p_loc would be set to NULL.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_location_info_delete(QBlockContext *context,
> +                             QBlockLocationInfo **p_loc);

Why are these functions necessary?  The user has the struct definitions
for QBlockLocationInfo so they can allocate/free them.

> +
> +/**
> + * qb_format_info_new: create a new QBlockFormatInfo structure.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @p_fmt: pointer that will receive created struct.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_format_info_new(QBlockContext *context,
> +                       QBlockFormatInfo **p_fmt);
> +
> +/**
> + * qb_format_info_delete: free QBlockFormatInfo structure.
> + *
> + * @context: operation context.
> + * @p_fmt: pointer to the struct, *p_fmt would be set to NULL.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_format_info_delete(QBlockContext *context,
> +                           QBlockFormatInfo **p_fmt);

Same here, I don't see a need for these functions.

> +
> +/**
> + * qb_open: open a block object.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + * @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 to NULL if you want to auto discovery the format.
> + * @flag: behavior control flags, it is LIBQBLOCK_O_XXX's combination.
> + *
> + * Note: For raw image, there is a risk that it's content is changed to some
> + *  magic value resulting a wrong probing done by libqblock, so don't do
> + * probing on raw images.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_open(QBlockContext *context,
> +            QBlockImage *qbi,
> +            QBlockLocationInfo *loc,
> +            QBlockFormatInfo *fmt,
> +            int flag);
> +
> +/**
> + * qb_close: close a block object.
> + *
> + * qb_flush is automatically done inside.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_close(QBlockContext *context,
> +              QBlockImage *qbi);
> +
> +/**
> + * 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.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + * @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.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_create(QBlockContext *context,
> +              QBlockImage *qbi,

Why is qbi required if the image isn't opened?  We should only need
context.

> +              QBlockLocationInfo *loc,
> +              QBlockFormatInfo *fmt,
> +              int flag);

What is the purpose of LIBQBLOCK_O_* flags if the image is not opened?

> +
> +
> +/* sync access */
> +/**
> + * qb_read: block sync read.
> + *
> + * return number of bytes read, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + * @buf: buffer that receive the content.
> + * @len: length to read.
> + * @offset: offset in the block data.
> + */
> +QEMU_DLL_PUBLIC
> +int32_t qb_read(QBlockContext *context,
> +                QBlockImage *qbi,
> +                uint8_t *buf,
> +                uint32_t len,
> +                uint64_t offset);

The uint32_t len and int32_t return types don't match up.  The return
value needs to be int64_t to handle the full uint32_t range.

However, the return value is only necessary if we want to do short
reads.  How about making the return value int with 0 on success and
negative on error?  Don't do short reads.

> +
> +/**
> + * qb_write: block sync write.
> + *
> + * return number of bytes written, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + * @buf: buffer that receive the content.
> + * @len: length to write.
> + * @offset: offset in the block data.
> + */
> +QEMU_DLL_PUBLIC
> +int32_t qb_write(QBlockContext *context,
> +                 QBlockImage *qbi,
> +                 const uint8_t *buf,
> +                 uint32_t len,
> +                 uint64_t offset);

Same issue with uint32_t len versus int32_t return types.

> +/**
> + * qb_flush: block sync flush.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_flush(QBlockContext *context,
> +             QBlockImage *qbi);
> +
> +
> +/* advance image APIs */
> +/**
> + * qb_check_allocation: check if [start, start+lenth-1] was allocated on the

s/lenth/length/

> + *  image.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + * @start: start position, unit is byte.
> + * @length: length to check, unit is byte, max is 1TB, otherwise will return
> + *   QB_ERR_INVALID_PARAM.
> + * @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(QBlockContext *context,
> +                        QBlockImage *qbi,
> +                        uint64_t start,
> +                        int64_t length,
> +                        int *pstatus,
> +                        int64_t *plength);

Why uint64_t start but int64_t length and *plength?  They should all be
unsigned.

> +
> +/* image information */
> +/**
> + * qb_get_image_info: get image info.
> + *
> + * return 0 on success, libqblock negative error value on fail.
> + *
> + * @context: operation context.
> + * @qbi: pointer to QBlockImage.
> + * @p_info: pointer that would receive the information.
> + *
> + * *info must be not modified after return, qb_info_image_static_delete will
> + *   use the information in it.
> + */
> +QEMU_DLL_PUBLIC
> +int qb_info_image_static_get(QBlockContext *context,
> +                             QBlockImage *qbi,
> +                             QBlockStaticInfo **p_info);
> +
> +/**
> + * qb_delete_image_info: free image info.
> + *
> + * @context: operation context.
> + * @p_info: pointer to the information struct, *p_info will be set to NULL.
> + */
> +QEMU_DLL_PUBLIC
> +void qb_info_image_static_delete(QBlockContext *context,
> +                                 QBlockStaticInfo **p_info);
> +
> +/* helper functions */
> +/**
> + * qb_str2fmttype: translate format string to libqblock format enum type.
> + *
> + * return the type, or QB_FORMAT_NONE if string matches none of supported types,
> + *  never return invalid value or QB_FORMAT_MAX.
> + *
> + * @fmt: the format string, if NULL it will return QB_FORMAT_NONE.
> + */
> +QEMU_DLL_PUBLIC
> +QBlockFormat qb_str2fmttype(const char *fmt_str);
> +
> +/**
> + * qb_formattype2str: translate libqblock format enum type to a string.
> + *
> + * return a pointer to the string, or NULL if type is not supported, and
> + *  returned pointer must NOT be freed.
> + *
> + * @fmt: the format enum type.
> + */
> +QEMU_DLL_PUBLIC
> +const char *qb_formattype2str(QBlockFormat fmt_type);

Why does the QBlockFormat enum exist?  Is there an issue with using strings?

> +/**
> + * qb_location_info_dup: duplicate a QBlockLocationInfo instance.
> + *
> + * return a pointer to new allocated one having the same values with input,
> + *  it need to be freed by qb_location_info_delete later. Never fail except OOM.
> + *
> + * @loc: pointer to the source instance.
> + */
> +QEMU_DLL_PUBLIC
> +QBlockLocationInfo *qb_location_info_dup(const QBlockLocationInfo *loc);
> +
> +/**
> + * qb_get_virt_size: get virtual size.
> + *
> + * return a pointer, which pointer to a member in info, or NULL if info is
> + *  not valid.
> + *
> + * @info: pointer to the QBlockStaticInfo structure.
> + */
> +QEMU_DLL_PUBLIC
> +const uint64_t *qb_get_virt_size(const QBlockStaticInfo *info);
> +
> +/**
> + * qb_get_backing_loc: get backing file location.
> + *
> + * return a pointer, which pointer to a member in info, or NULL if info is
> + *  not valid, or image have no such property.
> + *
> + * @info: pointer to the QBlockStaticInfo structure.
> + */
> +QEMU_DLL_PUBLIC
> +const QBlockLocationInfo *qb_get_backing_loc(const QBlockStaticInfo *info);
> +
> +/**
> + * qb_get_encrypt: get encrytion flag.
> + *
> + * return a pointer, which pointer to a member in info, or NULL if info is
> + *  not valid, or image have no such property.
> + *
> + * @info: pointer to the QBlockStaticInfo structure.
> + */
> +QEMU_DLL_PUBLIC
> +const bool *qb_get_encrypt(const QBlockStaticInfo *info);

These functions suggest the caller shouldn't look inside
QBLockStaticInfo although the struct definition seems to be public?

Stefan

Thank u for reviewing so many code.

>> +
>> +#ifndef LIBQBLOCK_ERROR
>> +#define LIBQBLOCK_ERROR
>> +
>> +#include "libqblock-types.h"
>> +
>> +#define QB_ERR_INTERNAL_ERR (-1)
>> +#define QB_ERR_FATAL_ERR (-2)
>> +#define QB_ERR_INVALID_PARAM (-100)
>> +#define QB_ERR_BLOCK_OUT_OF_RANGE (-101)
>> +
>> +/* error handling */
>> +/**
>> + * qb_error_get_human_str: get human readable error string.
>> + *
>> + * return a human readable string, it would be truncated if buf is not big
>> + *  enough.
>> + *
>> + * @context: operation context, must be valid.
>> + * @buf: buf to receive the string.
>> + * @buf_size: the size of the string buf.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_error_get_human_str(QBlockContext *context,
>> +                            char *buf, size_t buf_size);
>
> This function is broken.  There is no way to find out how big buf must
> be and there is no way to find if the result is truncated or not.
>
> This is actually worse than:
> #define QB_ERROR_HUMAN_SIZE 512
> /* buf must be QB_ERROR_HUMAN_SIZE */
> void qb_error_get_human_str(QBlockContext *context, char *buf);
>
> At least here the caller knows they will get the full error message
> because the library designer knows the error message lengths better than
> the application writer.
>
> The nicest would be:
>
> /* Return human-readable error message.  The caller must use free(3). */
> char *qb_error_get_human_str(QBlockContext *context);
>
   OK, I'll use this format.

>> +
>> +/**
>> + * qb_error_get_errno: get error number, only valid when err_ret is
>> + *   QB_ERR_INTERNAL_ERR.
>> + *
>> + * return negative errno if last error is QB_ERR_INTERNAL_ERR, otherwise 0.
>> + *
>> + * @context: operation context.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_error_get_errno(QBlockContext *context);
>
> QB_ERR_INTERNAL_ERR is an internal error which means the caller doesn't
> know exactly what happened.  How is the application supposed to use the
> errno?
>
> The only thing the application could do is strerror(3) but
> qb_error_get_human_str() should already do that.
>
> Therefore qb_error_get_errno() serves no purpose.
>
   will remove it.

>> +
>> +#endif
>> diff --git a/libqblock/libqblock-internal.h b/libqblock/libqblock-internal.h
>> new file mode 100644
>> index 0000000..91521f5
>> --- /dev/null
>> +++ b/libqblock/libqblock-internal.h
>> @@ -0,0 +1,68 @@

>> +
>> +#define G_LIBQBLOCK_ERROR g_libqblock_error_quark()
>> +
>> +static inline GQuark g_libqblock_error_quark(void)
>> +{
>> +    return g_quark_from_static_string("g-libqblock-error-quark");
>> +}
>
> qb_error_quark() would be a better name.  g_*() is for GLib not for code
> using glib.
>
> The shorter name also means you can drop the macro.  Macros like
> QB_FREE() and G_LIBQBLOCK_ERROR hide a very simple operation.  They
> force people reading the code to jump to its definition; it interrupts
> the reader and forces them to remember abstractions that add no value.
>
   OK.

>> +#endif
>> diff --git a/libqblock/libqblock-types.h b/libqblock/libqblock-types.h
>> index e69de29..d49b321 100644
>> --- a/libqblock/libqblock-types.h
>> +++ b/libqblock/libqblock-types.h
>> @@ -0,0 +1,245 @@
>> +/*
>> + * QEMU block layer library
>> + *
>> + * Copyright IBM, Corp. 2013
>> + *
>> + * 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>
>> +
>> +/*
>> + * In case libqblock is used by other project which have not defined
>> + * QEMU_DLL_PUBLIC.
>> + */
>> +#ifndef QEMU_DLL_PUBLIC
>> +#define QEMU_DLL_PUBLIC
>> +#endif
>> +
>> +/* this library is designed around this core struct. */
>> +typedef struct QBlockImage QBlockImage;
>> +
>> +/* 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)
>
> It is tempting to simplify the libqblock cache options so that:
>
>   * The host page cache is always used.
>   * Flush operations are required to ensure writes reach the disk.
>
> This means disk images would behave like regular files.  And it would
> prevent a lot of confusion about the meaning of the cache modes.
>
> However, advanced applications might want to use O_DIRECT or even
> NO_FLUSH.  So I guess it's okay to expose this.
>
   OK, will add those comments.

>> +
>> +typedef enum QBlockProtocol {
>> +    QB_PROTO_NONE = 0,
>> +    QB_PROTO_FILE,
>> +    QB_PROTO_MAX
>> +} QBlockProtocol;
>
> What prevents libqblock from supporting all protocols?
>
   I think no problem exist in supporting all protocols, it just
need more work to sort out the options in protocols, so removed
them in 1st version.

>> +typedef struct QBlockProtocolOptionsFile {
>> +    const char *filename;
>> +} QBlockProtocolOptionsFile;
>> +
>> +/**
>> + * struct QBlockLocationInfo: contains information about how to find the image
>> + *
>> + * @prot_type: protocol type, now only support FILE.
>> + * @o_file: file protocol related attributes.
>> + * @reserved: reserved bytes for ABI.
>> + */
>> +#define QBLOCK_PROT_OPTIONS_UNION_SIZE (512)
>> +typedef struct QBlockLocationInfo {
>> +    QBlockProtocol prot_type;
>> +    union {
>> +        QBlockProtocolOptionsFile o_file;
>> +        uint64_t reserved[QBLOCK_PROT_OPTIONS_UNION_SIZE/sizeof(uint64_t)];
>> +    };
>> +} QBlockLocationInfo;
>> +
>> +
>> +/* format related options */
>> +typedef enum QBlockFormat {
>> +    QB_FORMAT_NONE = 0,
>> +    QB_FORMAT_COW,
>> +    QB_FORMAT_QED,
>> +    QB_FORMAT_QCOW,
>> +    QB_FORMAT_QCOW2,
>> +    QB_FORMAT_RAW,
>> +    QB_FORMAT_RBD,
>> +    QB_FORMAT_SHEEPDOG,
>> +    QB_FORMAT_VDI,
>> +    QB_FORMAT_VMDK,
>> +    QB_FORMAT_VPC,
>> +    QB_FORMAT_MAX
>> +} QBlockFormat;
>> +
>> +typedef struct QBlockFormatOptionsCOW {
>> +    QBlockLocationInfo backing_loc;
>> +} QBlockFormatOptionsCOW;
>
> Are these struct definitions frozen?
>
> An application could do:
>
> QBlockFormatInfo info = ...;
> QBlockFormatOptionsCOW opts = info.o_cow; /* broken */
>
> If QBlockFormatOptionsCOW changes size then the application can break.
> It's hard to demand that applications don't use these structs directly
> and once broken applications rely on it we may have a hard time arguing
> with them that it's their fault the application is incompatible with new
> versions of libqblock (even if we're technically right).
>
   The size may grow with new member added in tail, and have different
cases:
1 user just switched the .so file.
   It is OK, new added member are ignored.
2 user recompile with new header and .so.
   It is OK, memory are adjusted.
   Both seems alright, maybe I missed something?

>> +typedef struct QBlockFormatOptionsQCOW2 {
>> +    QBlockLocationInfo backing_loc;
>> +    QBlockFormat backing_fmt;
>> +    bool encrypt;
>> +    uint64_t cluster_size; /* unit is bytes */
>> +    QBlockFormatOptionsQCOW2CompatLv cpt_lv;
>> +    QBlockFormatOptionsQCOW2PreAlloc pre_mode;
>> +} QBlockFormatOptionsQCOW2;
>
> ...this is an example of a struct where we still need to add fields.
> qcow2 is still under development and will get new options.
>
>> diff --git a/libqblock/libqblock.h b/libqblock/libqblock.h
>> index e69de29..d3fcaf5 100644
>> --- a/libqblock/libqblock.h
>> +++ b/libqblock/libqblock.h
>> @@ -0,0 +1,358 @@
>> +/*
>> + * QEMU block layer library
>> + *
>> + * Copyright IBM, Corp. 2013
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +/* Note: This library is not thread safe yet. */
>> +
>> +#ifndef LIBQBLOCK_H
>> +#define LIBQBLOCK_H
>> +
>> +#include "libqblock-types.h"
>> +#include "libqblock-error.h"
>> +
>> +/**
>> + * qb_context_new: allocate a new context.
>> + *
>> + * Broker is used to pass operation to libqblock, and get feedback from it.
>
> Broker?
>
   my bad, will fix.

>> + *
>> + * Returns 0 on success, libqblock negative error value on fail.
>> + *
>> + * Note this function will try init the library, so it must be called the
>> + * first.
>> + *
>> + * @p_context: used to receive the created struct.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_context_new(QBlockContext **p_context);
>> +
>> +/**
>> + * qb_context_delete: delete context.
>> + *
>> + * Broker will be freed and set to NULL.
>> + *
>> + * @p_context: pointer to the struct pointer, *p_context will be set to NULL.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_context_delete(QBlockContext **p_context);
>> +
>> +/**
>> + * qb_image_new: allocate a new QBlockImage struct.
>> + *
>> + * Subsequent qblock actions will use this struct, ref is set to 1.
>> + *
>> + * Returns 0 if succeed, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @p_qbi: used to receive the created struct.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_image_new(QBlockContext *context,
>> +                 QBlockImage **p_qbi);
>> +
>> +/**
>> + * qb_image_ref: increase the ref of QBlockImage.
>> + *
>> + * @context: operation context.
>> + * @p_qbi: pointer to the struct's pointer.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_image_ref(QBlockContext *context,
>> +                  QBlockImage **p_qbi);
>
> Why QBlockImage** instead of QBlockImage*?
>
   OK, will use QBlockImage*.

> What is the relationship between QBlockImage and its context?  Can I
> have two contexts A and B like this:
>
> qb_image_ref(ctx_a, &qbi);
> qb_image_unref(ctx_b, &qbi);
>
> Is this okay?
>
   it should be OK if block layer is thread safe in the future,
a thread should own a context. But caller may need to make sure
every context should call ref/unref as pairs.

>> +
>> +/**
>> + * qb_image_unref: decrease the ref of QBlockImage.
>> + *
>> + * if ref is reduced to 0, p_qbi would be freed and set to NULL, at which time
>> + *  if image was opened and qb_close was not called before, it would be
>> + *  automatically closed.
>> + *
>> + * @context: operation context.
>> + * @p_qbi: pointer to the struct's pointer.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_image_unref(QBlockContext *context,
>> +                    QBlockImage **p_qbi);
>> +
>> +/**
>> + * qb_location_info_new: create a new QBlockLocationInfo object.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @p_loc: pointer to receive the new created one.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_location_info_new(QBlockContext *context,
>> +                         QBlockLocationInfo **p_loc);
>> +
>> +/**
>> + * qb_location_info_delete: free a QBlockLocationInfo.
>> + *
>> + * @context: operation context.
>> + * @p_loc: pointer to the object, *p_loc would be set to NULL.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_location_info_delete(QBlockContext *context,
>> +                             QBlockLocationInfo **p_loc);
>
> Why are these functions necessary?  The user has the struct definitions
> for QBlockLocationInfo so they can allocate/free them.
>
   More likely a helper function. For ABI libqblock allocate
the struct instead of user, as a counter part free is also
handered by libqblock, to make sure the new added member
is not missed in free.

>> +
>> +/**
>> + * qb_format_info_new: create a new QBlockFormatInfo structure.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @p_fmt: pointer that will receive created struct.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_format_info_new(QBlockContext *context,
>> +                       QBlockFormatInfo **p_fmt);
>> +
>> +/**
>> + * qb_format_info_delete: free QBlockFormatInfo structure.
>> + *
>> + * @context: operation context.
>> + * @p_fmt: pointer to the struct, *p_fmt would be set to NULL.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_format_info_delete(QBlockContext *context,
>> +                           QBlockFormatInfo **p_fmt);
>
> Same here, I don't see a need for these functions.
>
>> +
>> +/**
>> + * qb_open: open a block object.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + * @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 to NULL if you want to auto discovery the format.
>> + * @flag: behavior control flags, it is LIBQBLOCK_O_XXX's combination.
>> + *
>> + * Note: For raw image, there is a risk that it's content is changed to some
>> + *  magic value resulting a wrong probing done by libqblock, so don't do
>> + * probing on raw images.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_open(QBlockContext *context,
>> +            QBlockImage *qbi,
>> +            QBlockLocationInfo *loc,
>> +            QBlockFormatInfo *fmt,
>> +            int flag);
>> +
>> +/**
>> + * qb_close: close a block object.
>> + *
>> + * qb_flush is automatically done inside.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_close(QBlockContext *context,
>> +              QBlockImage *qbi);
>> +
>> +/**
>> + * 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.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + * @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.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_create(QBlockContext *context,
>> +              QBlockImage *qbi,
>
> Why is qbi required if the image isn't opened?  We should only need
> context.
>
   Will remove.

>> +              QBlockLocationInfo *loc,
>> +              QBlockFormatInfo *fmt,
>> +              int flag);
>
> What is the purpose of LIBQBLOCK_O_* flags if the image is not opened?
>
   It is used for open backing up file, I'll add doc for it.

>> +
>> +
>> +/* sync access */
>> +/**
>> + * qb_read: block sync read.
>> + *
>> + * return number of bytes read, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + * @buf: buffer that receive the content.
>> + * @len: length to read.
>> + * @offset: offset in the block data.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int32_t qb_read(QBlockContext *context,
>> +                QBlockImage *qbi,
>> +                uint8_t *buf,
>> +                uint32_t len,
>> +                uint64_t offset);
>
> The uint32_t len and int32_t return types don't match up.  The return
> value needs to be int64_t to handle the full uint32_t range.
>
> However, the return value is only necessary if we want to do short
> reads.  How about making the return value int with 0 on success and
> negative on error?  Don't do short reads.
>
   I think change len to int32_t make more sense, this gives more
flexibilty, if we found the implement or API inside may return
a not completed operation.

>> +
>> +/**
>> + * qb_write: block sync write.
>> + *
>> + * return number of bytes written, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + * @buf: buffer that receive the content.
>> + * @len: length to write.
>> + * @offset: offset in the block data.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int32_t qb_write(QBlockContext *context,
>> +                 QBlockImage *qbi,
>> +                 const uint8_t *buf,
>> +                 uint32_t len,
>> +                 uint64_t offset);
>
> Same issue with uint32_t len versus int32_t return types.
>
>> +/**
>> + * qb_flush: block sync flush.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_flush(QBlockContext *context,
>> +             QBlockImage *qbi);
>> +
>> +
>> +/* advance image APIs */
>> +/**
>> + * qb_check_allocation: check if [start, start+lenth-1] was allocated on the
>
> s/lenth/length/
>
   OK.

>> + *  image.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + * @start: start position, unit is byte.
>> + * @length: length to check, unit is byte, max is 1TB, otherwise will return
>> + *   QB_ERR_INVALID_PARAM.
>> + * @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(QBlockContext *context,
>> +                        QBlockImage *qbi,
>> +                        uint64_t start,
>> +                        int64_t length,
>> +                        int *pstatus,
>> +                        int64_t *plength);
>
> Why uint64_t start but int64_t length and *plength?  They should all be
> unsigned.
>
   bdrv_is_allocated use in with sector unit, so used
int64_t with byte unit to simplify it, will change them.

>> +
>> +/* image information */
>> +/**
>> + * qb_get_image_info: get image info.
>> + *
>> + * return 0 on success, libqblock negative error value on fail.
>> + *
>> + * @context: operation context.
>> + * @qbi: pointer to QBlockImage.
>> + * @p_info: pointer that would receive the information.
>> + *
>> + * *info must be not modified after return, qb_info_image_static_delete will
>> + *   use the information in it.
>> + */
>> +QEMU_DLL_PUBLIC
>> +int qb_info_image_static_get(QBlockContext *context,
>> +                             QBlockImage *qbi,
>> +                             QBlockStaticInfo **p_info);
>> +
>> +/**
>> + * qb_delete_image_info: free image info.
>> + *
>> + * @context: operation context.
>> + * @p_info: pointer to the information struct, *p_info will be set to NULL.
>> + */
>> +QEMU_DLL_PUBLIC
>> +void qb_info_image_static_delete(QBlockContext *context,
>> +                                 QBlockStaticInfo **p_info);
>> +
>> +/* helper functions */
>> +/**
>> + * qb_str2fmttype: translate format string to libqblock format enum type.
>> + *
>> + * return the type, or QB_FORMAT_NONE if string matches none of supported types,
>> + *  never return invalid value or QB_FORMAT_MAX.
>> + *
>> + * @fmt: the format string, if NULL it will return QB_FORMAT_NONE.
>> + */
>> +QEMU_DLL_PUBLIC
>> +QBlockFormat qb_str2fmttype(const char *fmt_str);
>> +
>> +/**
>> + * qb_formattype2str: translate libqblock format enum type to a string.
>> + *
>> + * return a pointer to the string, or NULL if type is not supported, and
>> + *  returned pointer must NOT be freed.
>> + *
>> + * @fmt: the format enum type.
>> + */
>> +QEMU_DLL_PUBLIC
>> +const char *qb_formattype2str(QBlockFormat fmt_type);
>
> Why does the QBlockFormat enum exist?  Is there an issue with using strings?
>
   These fmt/enum code will always exist in an application to handler
different format, the choice is whether libqblock handle it for
the application. This is more a choice, my idea do it for user. What
is your idea?

>> +/**
>> + * qb_location_info_dup: duplicate a QBlockLocationInfo instance.
>> + *
>> + * return a pointer to new allocated one having the same values with input,
>> + *  it need to be freed by qb_location_info_delete later. Never fail except OOM.
>> + *
>> + * @loc: pointer to the source instance.
>> + */
>> +QEMU_DLL_PUBLIC
>> +QBlockLocationInfo *qb_location_info_dup(const QBlockLocationInfo *loc);
>> +
>> +/**
>> + * qb_get_virt_size: get virtual size.
>> + *
>> + * return a pointer, which pointer to a member in info, or NULL if info is
>> + *  not valid.
>> + *
>> + * @info: pointer to the QBlockStaticInfo structure.
>> + */
>> +QEMU_DLL_PUBLIC
>> +const uint64_t *qb_get_virt_size(const QBlockStaticInfo *info);
>> +
>> +/**
>> + * qb_get_backing_loc: get backing file location.
>> + *
>> + * return a pointer, which pointer to a member in info, or NULL if info is
>> + *  not valid, or image have no such property.
>> + *
>> + * @info: pointer to the QBlockStaticInfo structure.
>> + */
>> +QEMU_DLL_PUBLIC
>> +const QBlockLocationInfo *qb_get_backing_loc(const QBlockStaticInfo *info);
>> +
>> +/**
>> + * qb_get_encrypt: get encrytion flag.
>> + *
>> + * return a pointer, which pointer to a member in info, or NULL if info is
>> + *  not valid, or image have no such property.
>> + *
>> + * @info: pointer to the QBlockStaticInfo structure.
>> + */
>> +QEMU_DLL_PUBLIC
>> +const bool *qb_get_encrypt(const QBlockStaticInfo *info);
>
> These functions suggest the caller shouldn't look inside
> QBLockStaticInfo although the struct definition seems to be public?
>
   OK, will hide it.

> Stefan
>

On Fri, Feb 01, 2013 at 01:51:21PM +0800, Wenchao Xia wrote:
> >>+typedef enum QBlockProtocol {
> >>+    QB_PROTO_NONE = 0,
> >>+    QB_PROTO_FILE,
> >>+    QB_PROTO_MAX
> >>+} QBlockProtocol;
> >
> >What prevents libqblock from supporting all protocols?
> >
>   I think no problem exist in supporting all protocols, it just
> need more work to sort out the options in protocols, so removed
> them in 1st version.

Good to hear.

> >Are these struct definitions frozen?
> >
> >An application could do:
> >
> >QBlockFormatInfo info = ...;
> >QBlockFormatOptionsCOW opts = info.o_cow; /* broken */
> >
> >If QBlockFormatOptionsCOW changes size then the application can break.
> >It's hard to demand that applications don't use these structs directly
> >and once broken applications rely on it we may have a hard time arguing
> >with them that it's their fault the application is incompatible with new
> >versions of libqblock (even if we're technically right).
> >
>   The size may grow with new member added in tail, and have different
> cases:
> 1 user just switched the .so file.
>   It is OK, new added member are ignored.

No, it's not okay:

QBlockFormatOptionsCOW opts = old_info.o_cow;
QBlockFormatInfo new_info;
new_info.o_cow = opts; /* broken, only copies old fields */

http://davidz25.blogspot.de/2011/07/writing-c-library-part-5.html#abi-api-versioning
http://plan99.net/~mike/writing-shared-libraries.html

If you want to allow backwards-compatible changes to
QBlockFormatOptionsCOW in the future, then it needs to include padding.

> >What is the relationship between QBlockImage and its context?  Can I
> >have two contexts A and B like this:
> >
> >qb_image_ref(ctx_a, &qbi);
> >qb_image_unref(ctx_b, &qbi);
> >
> >Is this okay?
> >
>   it should be OK if block layer is thread safe in the future,
> a thread should own a context. But caller may need to make sure
> every context should call ref/unref as pairs.

Hmm...I still don't understand the relationship between QBlockImage and
a context.  Is it safe to use a QBlockImage with context B if it was
created with context A?  This should be documented.

It would be simpler if a QBlockImage is associated with a context.  Then
you can drop the context argument from functions that operate on a
QBlockImage.

If necessary for multi-threading, you can provide a function later that
associated a QBlockImage with a new context.  This allows applications
to use QBlockImage with more than one context.

> >>+/**
> >>+ * qb_location_info_delete: free a QBlockLocationInfo.
> >>+ *
> >>+ * @context: operation context.
> >>+ * @p_loc: pointer to the object, *p_loc would be set to NULL.
> >>+ */
> >>+QEMU_DLL_PUBLIC
> >>+void qb_location_info_delete(QBlockContext *context,
> >>+                             QBlockLocationInfo **p_loc);
> >
> >Why are these functions necessary?  The user has the struct definitions
> >for QBlockLocationInfo so they can allocate/free them.
> >
>   More likely a helper function. For ABI libqblock allocate
> the struct instead of user, as a counter part free is also
> handered by libqblock, to make sure the new added member
> is not missed in free.

We need to be very disciplined about struct definitions that are exposed
to applications.  There are no compiler warnings when an application
copies a libqblock struct, so we cannot prevent (bad) applications from
breaking when the struct changes.

Either provide the struct definition with padding and allow the
application to allocate/free it - then you don't need these helper
functions.

Or hide the struct and manage allocation/freeing and field access with
accessor functions.

If you try to mix these approaches the ABI will break when the library
changes.

> >>+/* sync access */
> >>+/**
> >>+ * qb_read: block sync read.
> >>+ *
> >>+ * return number of bytes read, libqblock negative error value on fail.
> >>+ *
> >>+ * @context: operation context.
> >>+ * @qbi: pointer to QBlockImage.
> >>+ * @buf: buffer that receive the content.
> >>+ * @len: length to read.
> >>+ * @offset: offset in the block data.
> >>+ */
> >>+QEMU_DLL_PUBLIC
> >>+int32_t qb_read(QBlockContext *context,
> >>+                QBlockImage *qbi,
> >>+                uint8_t *buf,
> >>+                uint32_t len,
> >>+                uint64_t offset);
> >
> >The uint32_t len and int32_t return types don't match up.  The return
> >value needs to be int64_t to handle the full uint32_t range.
> >
> >However, the return value is only necessary if we want to do short
> >reads.  How about making the return value int with 0 on success and
> >negative on error?  Don't do short reads.
> >
>   I think change len to int32_t make more sense, this gives more
> flexibilty, if we found the implement or API inside may return
> a not completed operation.

Kevin: Does the block layer do short reads/writes?

> >>+/**
> >>+ * qb_formattype2str: translate libqblock format enum type to a string.
> >>+ *
> >>+ * return a pointer to the string, or NULL if type is not supported, and
> >>+ *  returned pointer must NOT be freed.
> >>+ *
> >>+ * @fmt: the format enum type.
> >>+ */
> >>+QEMU_DLL_PUBLIC
> >>+const char *qb_formattype2str(QBlockFormat fmt_type);
> >
> >Why does the QBlockFormat enum exist?  Is there an issue with using strings?
> >
>   These fmt/enum code will always exist in an application to handler
> different format, the choice is whether libqblock handle it for
> the application. This is more a choice, my idea do it for user. What
> is your idea?

Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
string is very cheap and eliminates the need to convert between strings
and enums.  Dropping the enum also means one less thing to update when a
new image format is added.

Stefan

Il 01/02/2013 10:32, Stefan Hajnoczi ha scritto:
>>> What is the relationship between QBlockImage and its context?  Can I
>>> have two contexts A and B like this:
>>>
>>> qb_image_ref(ctx_a, &qbi);
>>> qb_image_unref(ctx_b, &qbi);
>>>
>>> Is this okay?
>>>
>>   it should be OK if block layer is thread safe in the future,
>> a thread should own a context. But caller may need to make sure
>> every context should call ref/unref as pairs.
> 
> Hmm...I still don't understand the relationship between QBlockImage and
> a context.  Is it safe to use a QBlockImage with context B if it was
> created with context A?  This should be documented.

No, it shouldn't.

> It would be simpler if a QBlockImage is associated with a context.  Then
> you can drop the context argument from functions that operate on a
> QBlockImage.

Yes, indeed.

> Either provide the struct definition with padding and allow the
> application to allocate/free it - then you don't need these helper
> functions.

We have so many structs and fields for the various formats, that our
choices are:

1) drown in a sea of accessors

2) rewrite the whole thing in Vala

3) add padding

I very much prefer (3), but (2) is only half joking.

> Or hide the struct and manage allocation/freeing and field access with
> accessor functions.
> 
> If you try to mix these approaches the ABI will break when the library
> changes.

> Kevin: Does the block layer do short reads/writes?

No, it doesn't.

>>>> +/**
>>>> + * qb_formattype2str: translate libqblock format enum type to a string.
>>>> + *
>>>> + * return a pointer to the string, or NULL if type is not supported, and
>>>> + *  returned pointer must NOT be freed.
>>>> + *
>>>> + * @fmt: the format enum type.
>>>> + */
>>>> +QEMU_DLL_PUBLIC
>>>> +const char *qb_formattype2str(QBlockFormat fmt_type);
>>>
>>> Why does the QBlockFormat enum exist?  Is there an issue with using strings?
>>>
>>   These fmt/enum code will always exist in an application to handler
>> different format, the choice is whether libqblock handle it for
>> the application. This is more a choice, my idea do it for user. What
>> is your idea?
> 
> Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
> string is very cheap and eliminates the need to convert between strings
> and enums.  Dropping the enum also means one less thing to update when a
> new image format is added.

Hmm, I've never seen discriminated records with strings.  When working
on the API with Wenchao, I tried to give it a QAPI flavor.

Paolo

On Tue, Feb 05, 2013 at 10:56:09AM +0100, Paolo Bonzini wrote:
> Il 01/02/2013 10:32, Stefan Hajnoczi ha scritto:
> >>> What is the relationship between QBlockImage and its context?  Can I
> >>> have two contexts A and B like this:
> >>>
> >>> qb_image_ref(ctx_a, &qbi);
> >>> qb_image_unref(ctx_b, &qbi);
> >>>
> >>> Is this okay?
> >>>
> >>   it should be OK if block layer is thread safe in the future,
> >> a thread should own a context. But caller may need to make sure
> >> every context should call ref/unref as pairs.
> > 
> > Hmm...I still don't understand the relationship between QBlockImage and
> > a context.  Is it safe to use a QBlockImage with context B if it was
> > created with context A?  This should be documented.
> 
> No, it shouldn't.

I guess you mean "It is not safe to use a QBlockImage with context B if
it was created with context A" rather than "No, it shouldn't be
documented"? :)

> > Kevin: Does the block layer do short reads/writes?
> 
> No, it doesn't.

Great, then we can use int return values where 0 means success and
-errno is used for errors.  No need to return the number of bytes from
the read/write functions.

> 
> >>>> +/**
> >>>> + * qb_formattype2str: translate libqblock format enum type to a string.
> >>>> + *
> >>>> + * return a pointer to the string, or NULL if type is not supported, and
> >>>> + *  returned pointer must NOT be freed.
> >>>> + *
> >>>> + * @fmt: the format enum type.
> >>>> + */
> >>>> +QEMU_DLL_PUBLIC
> >>>> +const char *qb_formattype2str(QBlockFormat fmt_type);
> >>>
> >>> Why does the QBlockFormat enum exist?  Is there an issue with using strings?
> >>>
> >>   These fmt/enum code will always exist in an application to handler
> >> different format, the choice is whether libqblock handle it for
> >> the application. This is more a choice, my idea do it for user. What
> >> is your idea?
> > 
> > Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
> > string is very cheap and eliminates the need to convert between strings
> > and enums.  Dropping the enum also means one less thing to update when a
> > new image format is added.
> 
> Hmm, I've never seen discriminated records with strings.  When working
> on the API with Wenchao, I tried to give it a QAPI flavor.

My thinking is that converting back and forth between string and enum is
a chore.  If we can reduce boilerplate the library becomes more pleasant
to use.

I guess it's nice to keep the structs QAPI compatible in case we want to
convert to QAPI later.

Stefan

Il 05/02/2013 13:39, Stefan Hajnoczi ha scritto:
> On Tue, Feb 05, 2013 at 10:56:09AM +0100, Paolo Bonzini wrote:
>> Il 01/02/2013 10:32, Stefan Hajnoczi ha scritto:
>>>>> What is the relationship between QBlockImage and its context?  Can I
>>>>> have two contexts A and B like this:
>>>>>
>>>>> qb_image_ref(ctx_a, &qbi);
>>>>> qb_image_unref(ctx_b, &qbi);
>>>>>
>>>>> Is this okay?
>>>>>
>>>>   it should be OK if block layer is thread safe in the future,
>>>> a thread should own a context. But caller may need to make sure
>>>> every context should call ref/unref as pairs.
>>>
>>> Hmm...I still don't understand the relationship between QBlockImage and
>>> a context.  Is it safe to use a QBlockImage with context B if it was
>>> created with context A?  This should be documented.
>>
>> No, it shouldn't.
> 
> I guess you mean "It is not safe to use a QBlockImage with context B if
> it was created with context A" rather than "No, it shouldn't be
> documented"? :)

Indeed. :)

>>>>>> +/**
>>>>>> + * qb_formattype2str: translate libqblock format enum type to a string.
>>>>>> + *
>>>>>> + * return a pointer to the string, or NULL if type is not supported, and
>>>>>> + *  returned pointer must NOT be freed.
>>>>>> + *
>>>>>> + * @fmt: the format enum type.
>>>>>> + */
>>>>>> +QEMU_DLL_PUBLIC
>>>>>> +const char *qb_formattype2str(QBlockFormat fmt_type);
>>>>>
>>>>> Why does the QBlockFormat enum exist?  Is there an issue with using strings?
>>>>>
>>>>   These fmt/enum code will always exist in an application to handler
>>>> different format, the choice is whether libqblock handle it for
>>>> the application. This is more a choice, my idea do it for user. What
>>>> is your idea?
>>>
>>> Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
>>> string is very cheap and eliminates the need to convert between strings
>>> and enums.  Dropping the enum also means one less thing to update when a
>>> new image format is added.
>>
>> Hmm, I've never seen discriminated records with strings.  When working
>> on the API with Wenchao, I tried to give it a QAPI flavor.
> 
> My thinking is that converting back and forth between string and enum is
> a chore.  If we can reduce boilerplate the library becomes more pleasant
> to use.

I hope/think in most case the user won't really need to deal with
format-specific options...

> I guess it's nice to keep the structs QAPI compatible in case we want to
> convert to QAPI later.

Yeah, or at least share idioms.

Paolo

Adding pad for every public structure seems the easiest way now,
I'll do that in next version.
> Il 05/02/2013 13:39, Stefan Hajnoczi ha scritto:
>> On Tue, Feb 05, 2013 at 10:56:09AM +0100, Paolo Bonzini wrote:
>>> Il 01/02/2013 10:32, Stefan Hajnoczi ha scritto:
>>>>>> What is the relationship between QBlockImage and its context?  Can I
>>>>>> have two contexts A and B like this:
>>>>>>
>>>>>> qb_image_ref(ctx_a, &qbi);
>>>>>> qb_image_unref(ctx_b, &qbi);
>>>>>>
>>>>>> Is this okay?
>>>>>>
>>>>>    it should be OK if block layer is thread safe in the future,
>>>>> a thread should own a context. But caller may need to make sure
>>>>> every context should call ref/unref as pairs.
>>>>
>>>> Hmm...I still don't understand the relationship between QBlockImage and
>>>> a context.  Is it safe to use a QBlockImage with context B if it was
>>>> created with context A?  This should be documented.
>>>
>>> No, it shouldn't.
>>
>> I guess you mean "It is not safe to use a QBlockImage with context B if
>> it was created with context A" rather than "No, it shouldn't be
>> documented"? :)
>
> Indeed. :)
>
   About the qbcontext, there are two choices:
1 discard of qbcontext, adding *error to get error message
for every API. Need to confirm how to make it work
with glib's aio-context.
2 keep qbcontext, every thread use it to store its thread
specific data, such as errors, and it is easy to package
glib's multi-thread contents inside.
   Which way do you like better?


>>>>>>> +/**
>>>>>>> + * qb_formattype2str: translate libqblock format enum type to a string.
>>>>>>> + *
>>>>>>> + * return a pointer to the string, or NULL if type is not supported, and
>>>>>>> + *  returned pointer must NOT be freed.
>>>>>>> + *
>>>>>>> + * @fmt: the format enum type.
>>>>>>> + */
>>>>>>> +QEMU_DLL_PUBLIC
>>>>>>> +const char *qb_formattype2str(QBlockFormat fmt_type);
>>>>>>
>>>>>> Why does the QBlockFormat enum exist?  Is there an issue with using strings?
>>>>>>
>>>>>    These fmt/enum code will always exist in an application to handler
>>>>> different format, the choice is whether libqblock handle it for
>>>>> the application. This is more a choice, my idea do it for user. What
>>>>> is your idea?
>>>>
>>>> Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
>>>> string is very cheap and eliminates the need to convert between strings
>>>> and enums.  Dropping the enum also means one less thing to update when a
>>>> new image format is added.
>>>
>>> Hmm, I've never seen discriminated records with strings.  When working
>>> on the API with Wenchao, I tried to give it a QAPI flavor.
>>
>> My thinking is that converting back and forth between string and enum is
>> a chore.  If we can reduce boilerplate the library becomes more pleasant
>> to use.
>
> I hope/think in most case the user won't really need to deal with
> format-specific options...
>
   Exposing the strings to user make libqblock thiner and easier, while
using enum instead of string, resulting a nicer API. The best thing I
can image is let qemu block layer use enum also to eliminate the
converting later, but effort may be not little. However, enum style
stands in the right direction, problem is the implemention, but it can
be hided inside, so I think it is OK.


>> I guess it's nice to keep the structs QAPI compatible in case we want to
>> convert to QAPI later.
>
> Yeah, or at least share idioms.
>
   Any special requirement in struct define to make it compatiable with
QAPI?


> Paolo
>

On Tue, Feb 05, 2013 at 11:00:11PM +0800, Wenchao Xia wrote:
>   Adding pad for every public structure seems the easiest way now,
> I'll do that in next version.
> >Il 05/02/2013 13:39, Stefan Hajnoczi ha scritto:
> >>On Tue, Feb 05, 2013 at 10:56:09AM +0100, Paolo Bonzini wrote:
> >>>Il 01/02/2013 10:32, Stefan Hajnoczi ha scritto:
> >>>>>>What is the relationship between QBlockImage and its context?  Can I
> >>>>>>have two contexts A and B like this:
> >>>>>>
> >>>>>>qb_image_ref(ctx_a, &qbi);
> >>>>>>qb_image_unref(ctx_b, &qbi);
> >>>>>>
> >>>>>>Is this okay?
> >>>>>>
> >>>>>   it should be OK if block layer is thread safe in the future,
> >>>>>a thread should own a context. But caller may need to make sure
> >>>>>every context should call ref/unref as pairs.
> >>>>
> >>>>Hmm...I still don't understand the relationship between QBlockImage and
> >>>>a context.  Is it safe to use a QBlockImage with context B if it was
> >>>>created with context A?  This should be documented.
> >>>
> >>>No, it shouldn't.
> >>
> >>I guess you mean "It is not safe to use a QBlockImage with context B if
> >>it was created with context A" rather than "No, it shouldn't be
> >>documented"? :)
> >
> >Indeed. :)
> >
>   About the qbcontext, there are two choices:
> 1 discard of qbcontext, adding *error to get error message
> for every API. Need to confirm how to make it work
> with glib's aio-context.
> 2 keep qbcontext, every thread use it to store its thread
> specific data, such as errors, and it is easy to package
> glib's multi-thread contents inside.
>   Which way do you like better?

What about associating a QBlockImage with a context?  Then errors can be
stored and retrieved from the context, but the user doesn't need to pass
the context explicitly.

Stefan

Il 05/02/2013 16:00, Wenchao Xia ha scritto:
>>> I guess you mean "It is not safe to use a QBlockImage with context B if
>>> it was created with context A" rather than "No, it shouldn't be
>>> documented"? :)
>>
>> Indeed. :)
>>
>   About the qbcontext, there are two choices:
> 1 discard of qbcontext, adding *error to get error message
> for every API. Need to confirm how to make it work
> with glib's aio-context.
> 2 keep qbcontext, every thread use it to store its thread
> specific data, such as errors, and it is easy to package
> glib's multi-thread contents inside.
>   Which way do you like better?

I agree with Stefan on this.

>>>>>>    These fmt/enum code will always exist in an application to handler
>>>>>> different format, the choice is whether libqblock handle it for
>>>>>> the application. This is more a choice, my idea do it for user. What
>>>>>> is your idea?
>>>>>
>>>>> Always use the strings ("qcow2", "raw", etc).  strcmp() on a 4 or 5-byte
>>>>> string is very cheap and eliminates the need to convert between strings
>>>>> and enums.  Dropping the enum also means one less thing to update when a
>>>>> new image format is added.
>>>>
>>>> Hmm, I've never seen discriminated records with strings.  When working
>>>> on the API with Wenchao, I tried to give it a QAPI flavor.
>>>
>>> My thinking is that converting back and forth between string and enum is
>>> a chore.  If we can reduce boilerplate the library becomes more pleasant
>>> to use.
>>
>> I hope/think in most case the user won't really need to deal with
>> format-specific options...
>
> Exposing the strings to user make libqblock thiner and easier

Not much because you have anyway to convert the option union to
QemuOption/QemuOptionParameter inside libqblock.  The only difference is
whether you use == or strcmp.

> , while
> using enum instead of string, resulting a nicer API.

Yes, I prefer the enum too.

> The best thing I
> can image is let qemu block layer use enum also to eliminate the
> converting later, but effort may be not little. However, enum style
> stands in the right direction, problem is the implemention, but it can
> be hided inside, so I think it is OK.
> 
> 
>>> I guess it's nice to keep the structs QAPI compatible in case we want to
>>> convert to QAPI later.
>>
>> Yeah, or at least share idioms.
>>
>   Any special requirement in struct define to make it compatiable with
> QAPI?

Not looking for strict compatibility, just having similar idioms is
fine.  What you're doing now is okay.

Paolo