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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/08/2012 12:12, Wenchao Xia ha scritto:
> +/* copy information and take care the member difference in differect version.
> +   Assuming all new member are added in the tail, struct size is the first
> +   member, this is old to new version, src have its struct_size set. */
> +static void qboc_adjust_o2n(struct QBlockOptionCreate *dest,
> +                            struct QBlockOptionCreate *src)
> +{
> +    /* for simple it does memcpy now, need to take care of embbed structure */
> +    memcpy(dest, src, src->struct_size);

You need an assertion that src->struct_size < sizeof(*dest).

However, the structure size perhaps wasn't my brightest idea ever.  But
still thanks for preparing this prototype!  Now that we have a more
complete API to discuss, we can iterate to something better.


> +        assert(0 == set_option_parameter_int(param,
> +                                BLOCK_OPT_SIZE, o_cow->virt_size));

Assertions should not have side effects.


> 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
> +
> +#define QB_ERR_MEM_ERR (-1)
> +#define QB_ERR_INTERNAL_ERR (-2)
> +#define QB_ERR_INVALID_PARAM (-3)
> +
> +/* 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);
> +
> +
> +/* 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

I'd rather avoid exposing this for now.

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

NATIVE_AIO should be an option for the file protocol.  But it's mostly
for performance and since we only support synchronous I/O we can drop it
for now.


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

I'd rather avoid exposing this for now too.

> +/* consistency hint for incoming migration */
> +#define LIBQBLOCK_O_INCOMING    0x0800

This is internal to QEMU.

Please add a mask of valid bits and an assertion that unknown bits are zero.

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

You are right that the embedded structs are very complicated.  I think
we need to find the right balance between structs for extensibility and
function arguments for ease of use.

For example, we could have

int qb_open(struct QBlockState *qbs,
            struct QBlockLocation *loc,
            struct QBlockFormatOption *op,
            int o_flag);

where:

- QBlockLocation is basically your QBlockLocInfo, but restructured to
use unions for the protocol-specific fields.

- QBlockFormatOption is basically your QBlockOptionFormat struct.  Thus
we can plan for specifying format-specific options at open time rather
than just at create time (this can be useful, for example, for things
such as lazy_refcounts).  Until support is there in the block layer, we
can simply check that all format-specific options are zero.

Since both QBlockLocation and QBlockFormatOption are basically a
discriminated record (enum + union of structs) we can add a large char
member to the union (e.g. char padding[512]) to keep the ABI stable.
Users must zero out all fields, and future additions must ensure that
the default value is represented by all-zeros.

> +    const char *prealloc_mode; /* off or metadata */

Make this an enum.

> +    bool prealloc_mode;

Here too.

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

Here too.

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

QBlockImageInfo

> +    int struct_size;

Here the struct size makes more sense, because it's a returned struct.

> +    char *filename;
> +    enum QBlockProtocol protocol;

Can this just be a struct QBlockLocation * (using a pointer avoids
problems with embedded structs)?

> +    enum QBlockFormat format;

Can this just be a struct QBlockFormatOption * (same as the above)?

> +    size_t virt_size;
> +    /* advance info */
> +    size_t allocated_size;
> +    bool encrypt;
> +    char *backing_filename;
> +};
> +
> +/* 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);

qb_get_image_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);

qb_free_image_info

Paolo

> +
> +/* 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);
> +#endif
>