Re: [Qemu-devel] [qemu-devel] [PATCH V2 2/3] [RFC] libqblock-API design

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

于 2012-8-10 19:02, Kevin Wolf 写道:
> Am 09.08.2012 12:12, schrieb Wenchao Xia:
>>    This patch is the API design.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   libqblock.c |  670 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   libqblock.h |  447 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 1117 insertions(+), 0 deletions(-)
>>   create mode 100644 libqblock.c
>>   create mode 100644 libqblock.h
>
> Ignoring the implementation for now as the design should be visible in
> the header file.
>
>> diff --git a/libqblock.h b/libqblock.h
>> new file mode 100644
>> index 0000000..d2e9502
>> --- /dev/null
>> +++ b/libqblock.h
>> @@ -0,0 +1,447 @@
>> +/*
>> + * Copyright IBM Corp. 2012
>> + *
>> + * Authors:
>> + *  Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
>> + */
>> +
>> +#ifndef LIBQBLOCK_H
>> +#define LIBQBLOCK_H
>> +
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <stdlib.h>
>> +
>> +#define bool _Bool
>
> Why not use stdbool.h?
>
   forgot to use that, will change it, thanks.


>> +
>> +#define QB_ERR_MEM_ERR (-1)
>> +#define QB_ERR_INTERNAL_ERR (-2)
>> +#define QB_ERR_INVALID_PARAM (-3)
>
> qemu uses errno values internally, I think they would make sense in the
> library interface as well.
>
>> +
>> +/* this library is designed around this core struct. */
>> +struct QBlockState;
>> +
>> +/*
>> +   libarary init
>> +   This function get the library ready to use.
>> + */
>> +void libqblock_init(void);
>> +
>> +/*
>> +   create a new qbs object
>> +   params:
>> +     qbs: out, pointer that will receive created obj.
>> +   return:
>> +     0 on succeed, negative on failure.
>> + */
>> +int qb_state_new(struct QBlockState **qbs);
>> +
>> +/*
>> +   delete a qbs object
>> +   params:
>> +     qbs: in, pointer that will be freed. *qbs will be set to NULL.
>> +   return:
>> +     void.
>> + */
>> +void qb_state_free(struct QBlockState **qbs);
>
> What happens if the qbs is open? Will it be flushed and closed? If so,
> can it fail and we need to allow an error return?
>
     user should call qb_close() if he have called qb_open(), other wise
unexpected thing happens such as this file is not flushed. I need
document this in the comments.

>> +
>> +/* flag used in open and create */
>> +#define LIBQBLOCK_O_RDWR        0x0002
>> +/* open the file read only and save writes in a snapshot */
>> +#define LIBQBLOCK_O_SNAPSHOT    0x0008
>> +/* do not use the host page cache */
>> +#define LIBQBLOCK_O_NOCACHE     0x0020
>> +/* use write-back caching */
>> +#define LIBQBLOCK_O_CACHE_WB    0x0040
>> +/* use native AIO instead of the thread pool */
>> +#define LIBQBLOCK_O_NATIVE_AIO  0x0080
>> +/* don't open the backing file */
>> +#define LIBQBLOCK_O_NO_BACKING  0x0100
>> +/* disable flushing on this disk */
>> +#define LIBQBLOCK_O_NO_FLUSH    0x0200
>> +/* copy read backing sectors into image */
>> +#define LIBQBLOCK_O_COPY_ON_READ 0x0400
>> +/* consistency hint for incoming migration */
>> +#define LIBQBLOCK_O_INCOMING    0x0800
>> +
>> +#define LIBQBLOCK_O_CACHE_MASK \
>> +   (LIBQBLOCK_O_NOCACHE | LIBQBLOCK_O_CACHE_WB | LIBQBLOCK_O_NO_FLUSH)
>> +
>> +enum QBlockProtocol {
>> +    QB_PROTO_FILE = 0,
>> +    QB_PROTO_MAX
>> +};
>> +
>> +enum QBlockFormat {
>> +    QB_FMT_NONE = 0,
>> +    QB_FMT_COW,
>> +    QB_FMT_QED,
>> +    QB_FMT_QCOW,
>> +    QB_FMT_QCOW2,
>> +    QB_FMT_RAW,
>> +    QB_FMT_RBD,
>> +    QB_FMT_SHEEPDOG,
>> +    QB_FMT_VDI,
>> +    QB_FMT_VMDK,
>> +    QB_FMT_VPC,
>> +    QB_FMT_MAX
>> +};
>
> Not sure if this is a good idea with respect to extensibility. Today you
> only need to create a new block/foo.c and add it to the Makefile in
> order to add a new format and protocol. It would be better if the
> library could make use of it without changing these enums, e.g. by
> referring to formats by string (possibly getting a struct referring to a
> format by something like qblk_get_format("raw"))
>
   qblk_get_format("raw") seems a good way solve the option difference,
maybe user call it as:
   void *op = qblk_get_format_option("raw");
   struct QBlockOption_raw *o_raw = (struct QBlockOption_raw *)op;
   o_raw.virt_size = 16 * 1024;

   I think this is good, with the cost of user have to deal string
instead of simpler enum integer.

>> +
>> +/* block target location info, it include all information about how to find
>> +   the image */
>> +struct QBlockLocInfo {
>> +    int struct_size;
>> +    const char *filename;
>> +    enum QBlockProtocol protocol;
>> +};
>
> This is a relatively simplistic view of the world. It works okay for
> local image files (which is probably the most common use case for the
> library), but you get into trouble as soon as you want to build some
> less trivial structures, like something including blkdebug.
>
> Eventually, this leads straight into -blockdev discussions, so maybe we
> should indeed go with a very simplistic approach for the start, but
> always be aware that a more general solution is to be expected and
> should be possible to fit in naturally.
>
   if blkdebug is used for debug block I/O, maybe we can sort it into
another protocol.

>> +
>> +/* how to open the image */
>> +struct QBlockOptionOpen {
>> +    int struct_size;
>> +    struct QBlockLocInfo o_loc; /* how to find */
>> +    enum QBlockFormat o_fmt_type; /* how to extract */
>> +    int o_flag; /* how to control */
>> +};
>> +
>> +/*
>> +   create a new QBlockOptionOpen structure.
>> +   params:
>> +     op: out, pointer that will receive created structure.
>> +   return:
>> +     0 on succeed, negative on failure.
>> + */
>> +static inline int qb_oo_new(struct QBlockOptionOpen **op)
>> +{
>> +    *op = calloc(1, sizeof(struct QBlockOptionOpen));
>> +    if (*op == NULL) {
>> +        return QB_ERR_MEM_ERR;
>> +    }
>> +    (*op)->struct_size = sizeof(struct QBlockOptionOpen);
>> +    (*op)->o_loc.struct_size = sizeof(struct QBlockLocInfo);
>> +    return 0;
>> +}
>> +
>> +/*
>> +   free QBlockOptionOpen structure.
>> +   params:
>> +     op: in, *op will be set as NULL after called.
>> +   return:
>> +     void
>> + */
>> +static inline void qb_oo_free(struct QBlockOptionOpen **op)
>> +{
>> +    free(*op);
>> +    *op = NULL;
>> +}
>> +
>> +/*
>> +   open a block object.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +     op: in, options for open.
>> +   return:
>> +     0 on success, negative on fail.
>> + */
>> +int qb_open(struct QBlockState *qbs, struct QBlockOptionOpen *op);
>
> No further explanation on what negative values there are allowed? In
> practice I guess -errno, but will it be officially undefined?
>
> Will there be a function that allows getting an error message if it has
> failed?
>
     Yes, there is a API doint that.

>> +
>> +/*
>> +   close a block object.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +   return:
>> +     void.
>> + */
>> +void qb_close(struct QBlockState *qbs);
>
> Does it imply a flush? Do we need an error return then?
>
    No flush now, will flush and document it next time.

>> +
>> +/* format related options, struct_size must be set. */
>> +struct QBlockOption_cow {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *backing_file;
>> +    int backing_flag;
>> +};
>> +
>> +struct QBlockOption_qed {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *backing_file;
>> +    enum QBlockFormat backing_fmt;
>> +    int backing_flag;
>> +    size_t cluster_size; /* unit is bytes */
>> +    size_t table_size; /* unit is clusters */
>> +};
>> +
>> +struct QBlockOption_qcow {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *backing_file;
>> +    int backing_flag;
>> +    bool encrypt;
>> +};
>> +
>> +struct QBlockOption_qcow2 {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *compat_level; /* "Compatibility level (0.10 or 1.1)" */
>> +    const char *backing_file;
>> +    enum QBlockFormat backing_fmt;
>> +    int backing_flag;
>> +    bool encrypt;
>> +    size_t cluster_size; /* unit is bytes */
>> +    const char *prealloc_mode; /* off or metadata */
>> +};
>> +
>> +struct QBlockOption_raw {
>> +    int struct_size;
>> +    size_t virt_size;
>> +};
>> +
>> +struct QBlockOption_rbd {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    size_t cluster_size;
>> +};
>> +
>> +struct QBlockOption_sheepdog {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *backing_file;
>> +    int backing_flag;
>> +    const char *prealloc_mode; /* off or full */
>> +};
>> +
>> +struct QBlockOption_vdi {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    size_t cluster_size;
>> +    bool prealloc_mode;
>> +};
>> +
>> +struct QBlockOption_vmdk {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *backing_file;
>> +    int backing_flag;
>> +    bool compat_version6;
>> +    const char *subfmt;
>> +    /* vmdk flat extent format, values:
>> +   "{monolithicSparse (default) | monolithicFlat | twoGbMaxExtentSparse |
>> +    twoGbMaxExtentFlat | streamOptimized} */
>> +};
>> +
>> +struct QBlockOption_vpc {
>> +    int struct_size;
>> +    size_t virt_size;
>> +    const char *subfmt; /* "{dynamic (default) | fixed} " */
>> +};
>
> Ouch. :-)
>
> But maybe it's the best we can do...
>
   This structure brought format specified options to the surface,
user could understand them easier and simply give values to the
members, so this is a friendly API. But with a union implement became
hard, I think the way your suggested as
qblk_format_option_get("raw")
could make thing easier.

>> +
>> +union QBlockOption_fmt {
>> +    struct QBlockOption_cow       o_cow;
>> +    struct QBlockOption_qed       o_qed;
>> +    struct QBlockOption_qcow      o_qcow;
>> +    struct QBlockOption_qcow2     o_qcow2;
>> +    struct QBlockOption_raw       o_raw;
>> +    struct QBlockOption_rbd       o_rbd;
>> +    struct QBlockOption_sheepdog  o_sheepdog;
>> +    struct QBlockOption_vdi       o_vdi;
>> +    struct QBlockOption_vmdk      o_vmdk;
>> +    struct QBlockOption_vpc       o_vpc;
>> +};
>> +
>> +struct QBlockOptionFormat {
>> +    int struct_size;
>> +    enum QBlockFormat fmt_type;
>> +    union QBlockOption_fmt fmt_op;
>> +};
>> +
>> +/* struct_size in o_loc and o_fmt must set. To make this structure extensible,
>> +   all new member must be added in the tail of each structure. */
>> +struct QBlockOptionCreate {
>> +    int struct_size;
>
> size_t?
>
   struct_size will not be big, should I also use size_t?

>> +    struct QBlockLocInfo o_loc;
>> +    struct QBlockOptionFormat o_fmt;
>> +};
>> +
>> +/*
>> +   create a new QBlockOptionCreate structure.
>> +   params:
>> +     op: out, pointer that will receive created structure.
>> +     fmt: format want to use.
>> +   return:
>> +     0 on succeed, negative on failure.
>> + */
>> +static inline int qb_oc_new(struct QBlockOptionCreate **op,
>> +                            enum QBlockFormat fmt)
>> +{
>> +    *op = calloc(1, sizeof(struct QBlockOptionCreate));
>> +    if (*op == NULL) {
>> +        return QB_ERR_MEM_ERR;
>> +    }
>> +    (*op)->struct_size = sizeof(struct QBlockOptionCreate);
>> +    (*op)->o_loc.struct_size = sizeof(struct QBlockLocInfo);
>> +    (*op)->o_fmt.struct_size = sizeof(struct QBlockOptionFormat);
>> +
>> +    switch (fmt) {
>> +    case QB_FMT_COW:
>> +        (*op)->o_fmt.fmt_op.o_cow.struct_size =
>> +                                          sizeof(struct QBlockOption_cow);
>> +        break;
>> +    case QB_FMT_QED:
>> +        (*op)->o_fmt.fmt_op.o_qed.struct_size =
>> +                                          sizeof(struct QBlockOption_qed);
>> +        break;
>> +    case QB_FMT_QCOW:
>> +        (*op)->o_fmt.fmt_op.o_qcow.struct_size =
>> +                                          sizeof(struct QBlockOption_qcow);
>> +        break;
>> +    case QB_FMT_QCOW2:
>> +        (*op)->o_fmt.fmt_op.o_qcow2.struct_size =
>> +                                          sizeof(struct QBlockOption_qcow2);
>> +        break;
>> +    case QB_FMT_RAW:
>> +        (*op)->o_fmt.fmt_op.o_raw.struct_size =
>> +                                          sizeof(struct QBlockOption_raw);
>> +        break;
>> +    case QB_FMT_RBD:
>> +        (*op)->o_fmt.fmt_op.o_rbd.struct_size =
>> +                                          sizeof(struct QBlockOption_rbd);
>> +        break;
>> +    case QB_FMT_SHEEPDOG:
>> +        (*op)->o_fmt.fmt_op.o_sheepdog.struct_size =
>> +                                          sizeof(struct QBlockOption_sheepdog);
>> +        break;
>> +    case QB_FMT_VDI:
>> +        (*op)->o_fmt.fmt_op.o_vdi.struct_size =
>> +                                          sizeof(struct QBlockOption_vdi);
>> +        break;
>> +    case QB_FMT_VMDK:
>> +        (*op)->o_fmt.fmt_op.o_vmdk.struct_size =
>> +                                          sizeof(struct QBlockOption_vmdk);
>> +        break;
>> +    case QB_FMT_VPC:
>> +        (*op)->o_fmt.fmt_op.o_vpc.struct_size =
>> +                                          sizeof(struct QBlockOption_vpc);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +    (*op)->o_fmt.fmt_type = fmt;
>> +    return 0;
>> +}
>> +
     This is what make implement hard, with format union, in order to
form a friendly API.


>> +/*
>> +   free QBlockOptionCreate structure.
>> +   params:
>> +     op: in, *op will be set as NULL after called.
>> +   return:
>> +     void
>> + */
>> +static inline void qb_oc_free(struct QBlockOptionCreate **op)
>> +{
>> +    free(*op);
>> +    *op = NULL;
>> +}
>> +/*
>> +   create a block file.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +     op: in, create option.
>> +   return:
>> +     negative on fail, 0 on success.
>> + */
>> +int qb_create(struct QBlockState *qbs, struct QBlockOptionCreate *op);
>> +
>> +
>> +/* sync access */
>> +/*
>> +   block sync read.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +     buf: out, buffer that receive the content.
>> +     len: in, length to read.
>> +     offset: in, offset in the block data.
>> +   return:
>> +     negative on fail, 0 on success.
>> + */
>> +int qb_read(struct QBlockState *qbs, const void *buf, size_t len,
>> +                                                      off_t offset);
>> +
>> +/*
>> +   block sync read.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +     buf: in, buffer that would be wrote.
>> +     len: in, length to write.
>> +     offset: in, offset in the block data.
>> +   return:
>> +     negative on fail, 0 on success.
>> + */
>> +int qb_write(struct QBlockState *qbs, const void *buf, size_t len,
>> +                                                       off_t offset);
>> +
>> +/*
>> +   block sync flush.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +   return:
>> +     negative on fail, 0 on success.
>> + */
>> +int qb_flush(struct QBlockState *qbs);
>> +
>> +/* information */
>> +/* image related info, static information, from user perspective. */
>> +/* now it is a plain structure, wonder if it could be foldered into embbed one
>> +   to reflect that format related information better. */
>> +struct QBlockInfoImage {
>> +    int struct_size;
>> +    char *filename;
>> +    enum QBlockProtocol protocol;
>> +    enum QBlockFormat format;
>> +    size_t virt_size;
>> +    /* advance info */
>> +    size_t allocated_size;
>> +    bool encrypt;
>> +    char *backing_filename;
>> +};
>
> What about things like cluster size, dirty state etc. that only some
> image formats have?
>
    hard to resolve, enum plus sub format structure seems reasonable. My
plan is using embbed structure:
struct QBlockInfoImage {
    int struct_size;
    struct QBlockInfoLoc *i_loc;
    struct QBlockInfoFormat *i_fmt;
};
   with format string user got, user do a cast to specified format info.

>> +
>> +/* image info */
>> +/*
>> +   get image info.
>> +   params:
>> +     qbs: in, pointer to QBlockState.
>> +     info, out, pointer that would receive the information.
>> +   return:
>> +     negative on fail, 0 on success.
>> + */
>> +int qb_infoimage_get(struct QBlockState *qbs, struct QBlockInfoImage **info);
>> +
>> +/*
>> +   free image info.
>> +   params:
>> +     info, in, pointer, *info would be set to NULL after function called.
>> +   return:
>> +     void.
>> + */
>> +void qb_infoimage_free(struct QBlockInfoImage **info);
>> +
>> +/* misc */
>> +bool qb_supports_format(enum QBlockFormat fmt);
>> +bool qb_supports_protocol(enum QBlockProtocol proto);
>> +
>> +const char *qb_error_get_detail(struct QBlockState *qbs, int *err_num);
>
> This needs a more detailed description. I also think that you'll want to
> add a const char** parameter for human readable error messages (as
> printed with qerror_report or error_set in qemu).
>
   This API try to return human readable strings. Will add document.

> Kevin
>


-- 
Best Regards

Wenchao Xia