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

[Kevin Wolf]

Am 09.08.2012 12:12, schrieb Wenchao Xia:
>   This patch is the API design.
> 
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  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 <address@hidden>
> + *
> + * 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?

> +
> +#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?

> +
> +/* 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"))

> +
> +/* 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.

> +
> +/* 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?

> +
> +/*
> +   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?

> +
> +/* 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...

> +
> +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 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;
> +}
> +
> +/*
> +   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?

> +
> +/* 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).

Kevin