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

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

于 2012-8-10 18:48, Paolo Bonzini 写道:
> 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.
>
    what side effects would this line have?

>
>> 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.
>

   OK to hide this flag now. Do you think this flag belongs to the
control flag or I should sort them  to anther option? In long term the
library should provide full capablities as qemu block layer have so
qemu can try migrate to it, so now I need to make sure the API designed
  have left room for all features qemu have support.

>> +
>> +#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)?
>
   Yes, I tried it before but found that structure alignment is hard to
solve, but as you suggest, I think plain structure plus pointer could
make it simpler. Blue Swirl suggest using a API to unify property
setting and solve version difference, not using structure, as:
  qb_set_property(s, "filename", "c:\\\\autoexec.bat");
  What do you think of that way?
>> +    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
>
   OK.

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


-- 
Best Regards

Wenchao Xia