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

[Wenchao Xia]

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   <address@hidden>
> > + *
> > + * 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   <address@hidden>
> > + *
> > + * 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