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

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

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
>


-- 
Best Regards

Wenchao Xia