[1/6] libqblock APIs

This patch contains the major APIs in the library.
Important APIs:
  1 QBroker. These structure was used to retrieve errors, every thread must
create one first, Later maybe thread related staff could be added into it.
  2 QBlockState. It stands for an block image object.
  3 QBlockInfoImageStatic. Now it is not folded with location and format.
  4 ABI was kept with reserved members.

structure, because it would cause caller more codes to find one member.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 libqblock/libqblock.c |  859 +++++++++++++++++++++++++++++++++++++++++++++++++
 libqblock/libqblock.h |  251 ++++++++++++++
 2 files changed, 1110 insertions(+), 0 deletions(-)
 create mode 100644 libqblock/libqblock.c
 create mode 100644 libqblock/libqblock.h

Il 03/09/2012 11:18, Wenchao Xia ha scritto:
>   1 QBroker. These structure was used to retrieve errors, every thread must
> create one first, Later maybe thread related staff could be added into it.

Can you use GError instead?

>   3 QBlockInfoImageStatic. Now it is not folded with location and format.

What does "Static" mean?

Paolo

On 09/03/2012 03:18 AM, Wenchao Xia wrote:
>   This patch contains the major APIs in the library.
> Important APIs:
>   1 QBroker. These structure was used to retrieve errors, every thread must
> create one first, Later maybe thread related staff could be added into it.
>   2 QBlockState. It stands for an block image object.
>   3 QBlockInfoImageStatic. Now it is not folded with location and format.
>   4 ABI was kept with reserved members.
> 
> structure, because it would cause caller more codes to find one member.

Commit message snafu?

> +/**
> + * libqblock_init: Initialize the library
> + */
> +void libqblock_init(void);

Is this function safe to call more than once?  Even tighter, is it safe
to call this function simultaneously from multiple threads?

> +
> +/**
> + * qb_broker_new: allocate a new broker
> + *
> + * Broker is used to pass operation to libqblock, and got feed back from it.

s/got feed back/get feedback/

> + *
> + * Returns 0 on success, negative value on fail.

Is there any particular interpretation to this negative value, such as
negative errno constant, or always -1?

> +
> +/**
> + * qb_state_new: allocate a new QBloctState struct

s/Bloct/Block/

> + *
> + * Following qblock action were based on this struct

Didn't read well.  Did you mean:

Subsequent qblock actions will use this struct

> + *
> + * Returns 0 if succeed, negative value on fail.

Again, is there any particular meaning to which negative value is returned?

> +
> +/**
> + * qb_open: open a block object.
> + *
> + * return 0 on success, negative on fail.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @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 NULL if you want auto discovery the format.

set to NULL if you want to auto-discover the format

Maybe also add a warning about the inherent security risks of attempting
format auto-discovery (any raw image must NOT be probed, as the raw
image can emulate any other format and cause qemu to chase down chains
where it should not).

> + * @flag: behavior control flags.

What flags are currently defined?

> + */
> +int qb_open(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            struct QBlockOptionLoc *loc,
> +            struct QBlockOptionFormat *fmt,
> +            int flag);
> +
> +/**
> + * qb_close: close a block object.
> + *
> + * qb_flush is automaticlly done inside.

s/automaticlly/automatically/

> +/**
> + * qb_create: create a block image or object.
> + *
> + * Note: Create operation would not open the image automatically.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @loc: location options for open, how to find the image.
> + * @fmt: format options, how to extract the data.
> + * @flag: behavior control flags.

Again, what flags are defined?

> +
> +/* sync access */
> +/**
> + * qb_read: block sync read.
> + *
> + * return negative on fail, 0 on success.

Shouldn't this instead return the number of successfully read bytes, to
allow for short reads if offset exceeds end-of-file?  If so, should it
return ssize_t instead of int?

> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @buf: buffer that receive the content.

s/receive/receives/

> + * @len: length to read.

Is there a magic length for reading the entire file in one go?

> + * @offset: offset in the block data.
> + */
> +int qb_read(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            const void *buf,
> +            size_t len,
> +            off_t offset);

You do realize that size_t and off_t are not necessarily the same width;
but I'm okay with limiting to size_t reads.

> +/**
> + * qb_write: block sync write.
> + *
> + * return negative on fail, 0 on success.

Again, this should probably return number of successfully written bytes,
as an ssize_t.

> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @buf: buffer that receive the content.

s/receive/supplies/

> +/* advance image APIs */
> +/**
> + * qb_is_allocated: check if following sectors was allocated on the image.
> + *
> + * return negative on fail, 0 or 1 on success. 0 means unallocated, 1 means
> + *allocated.

Formatting is off.

> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @sector_num: start sector number. If 'sector_num' is beyond the end of the
> + *disk image the return value is 0 and 'pnum' is set to 0.
> + * @nb_sectors: how many sector would be checked, it is the max value 'pnum'
> + *should be set to.  If nb_sectors goes beyond the end of the disk image it
> + *will be clamped.
> + * @pnum: pointer to receive how many sectors are allocated or unallocated.

This interface requires the user to know how big a sector is.  Would it
be any more convenient to the user to pass offsets, rather than sector
numbers; and/or have a function for converting between offsets and
sector numbers?

> + */
> +int qb_is_allocated(struct QBroker *broker,
> +                    struct QBlockState *qbs,
> +                    int64_t sector_num,
> +                    int nb_sectors,

Shouldn't nb_sectors be size_t?

> +                    int *pnum);

Exactly how does the *pnum argument work?  This interface looks like it
isn't fully thought out yet.  Either I want to know if a chunk of
sectors is allocated (I supply start and length of sectors to check),
regardless of how many sectors beyond that point are also allocated
(pnum makes no sense); or I want to know how many sectors are allocated
from a given point (I supply start, and the function returns length, so
nb_sectors makes no sense).  Either way, I think you are supplying too
many parameters for how I envision checking for allocated sectors.

Il 03/09/2012 15:56, Eric Blake ha scritto:
> Exactly how does the *pnum argument work?  This interface looks like it
> isn't fully thought out yet.  Either I want to know if a chunk of
> sectors is allocated (I supply start and length of sectors to check),
> regardless of how many sectors beyond that point are also allocated
> (pnum makes no sense);

pnum makes sense if the [start, start+length) range includes both
allocated and unallocated sectors.

> or I want to know how many sectors are allocated
> from a given point (I supply start, and the function returns length, so
> nb_sectors makes no sense).

This operation could be O(number of blocks in disk) worst case, so it
makes sense to provide nb_sectors as an upper bound.  nb_sectors is
typically dictated by the size of your buffer.

That said, QEMU's internal bdrv_is_allocated function does have one not
entirely appealing property: the block at start + *pnum might have the
same state as the block at start + *pnum - 1, even if *pnum < length.
We may want to work around this in libqblock, but we could also simply
document it.

Paolo

> Either way, I think you are supplying too
> many parameters for how I envision checking for allocated sectors.

On Mon, Sep 3, 2012 at 9:18 AM, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>   This patch contains the major APIs in the library.
> Important APIs:
>   1 QBroker. These structure was used to retrieve errors, every thread must
> create one first, Later maybe thread related staff could be added into it.
>   2 QBlockState. It stands for an block image object.
>   3 QBlockInfoImageStatic. Now it is not folded with location and format.
>   4 ABI was kept with reserved members.
>
> structure, because it would cause caller more codes to find one member.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  libqblock/libqblock.c |  859 +++++++++++++++++++++++++++++++++++++++++++++++++
>  libqblock/libqblock.h |  251 ++++++++++++++
>  2 files changed, 1110 insertions(+), 0 deletions(-)
>  create mode 100644 libqblock/libqblock.c
>  create mode 100644 libqblock/libqblock.h
>
> diff --git a/libqblock/libqblock.c b/libqblock/libqblock.c
> new file mode 100644
> index 0000000..3983802
> --- /dev/null
> +++ b/libqblock/libqblock.c
> @@ -0,0 +1,859 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Wenchao Xia   <xiawenc@linux.vnet.ibm.com>
> + *
> + * 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.
> + *
> + */
> +
> +#include <unistd.h>
> +#include <stdarg.h>
> +
> +#include "libqblock.h"
> +#include "libqblock-helper.h"
> +
> +#include "qemu-aio.h"
> +
> +void libqblock_init(void)
> +{
> +    bdrv_init();
> +    qemu_init_main_loop();
> +}
> +
> +int qb_broker_new(struct QBroker **broker)
> +{
> +    *broker = FUNC_CALLOC(1, sizeof(struct QBroker));
> +    if (*broker == NULL) {
> +        return QB_ERR_MEM_ERR;
> +    }
> +    return 0;
> +}
> +
> +void qb_broker_delete(struct QBroker **broker)
> +{
> +    CLEAN_FREE(*broker);
> +    return;
> +}
> +
> +int qb_state_new(struct QBroker *broker,
> +                 struct QBlockState **qbs)
> +{
> +    *qbs = FUNC_CALLOC(1, sizeof(struct QBlockState));
> +    if (*qbs == NULL) {
> +        set_broker_err_nomem(broker);
> +        return broker->err_ret;
> +    }
> +    (*qbs)->bdrvs = bdrv_new("hda");
> +    if ((*qbs)->bdrvs == NULL) {
> +        CLEAN_FREE(*qbs);
> +        set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                       "failed to create the driver.");
> +        return broker->err_ret;
> +    }
> +    return 0;
> +}
> +
> +void qb_state_delete(struct QBroker *broker,
> +                     struct QBlockState **qbs)
> +{
> +    CLEAN_FREE((*qbs)->filename);
> +    if ((*qbs)->bdrvs != NULL) {
> +        bdrv_delete((*qbs)->bdrvs);
> +        (*qbs)->bdrvs = NULL;
> +    }
> +    CLEAN_FREE(*qbs);
> +    return;
> +}
> +
> +int qb_ol_new(struct QBroker *broker,
> +              struct QBlockOptionLoc **op)
> +{
> +    *op = FUNC_CALLOC(1, sizeof(struct QBlockOptionLoc));
> +    if (*op == NULL) {
> +        set_broker_err_nomem(broker);
> +        return broker->err_ret;
> +    }
> +    return 0;
> +}
> +
> +void qb_ol_delete(struct QBroker *broker,
> +                  struct QBlockOptionLoc **op)
> +{
> +    CLEAN_FREE(*op);
> +}
> +
> +int qb_of_new(struct QBroker *broker,
> +              struct QBlockOptionFormat **op)
> +{
> +    *op = FUNC_CALLOC(1, sizeof(struct QBlockOptionFormat));
> +    if (*op == NULL) {
> +        set_broker_err_nomem(broker);
> +        return broker->err_ret;
> +    }
> +    return 0;
> +}
> +
> +void qb_of_delete(struct QBroker *broker,
> +                  struct QBlockOptionFormat **op)
> +{
> +    CLEAN_FREE(*op);
> +}
> +
> +/* return 0 if every thing is fine */
> +static int loc_check_params(struct QBroker *broker,
> +                            struct QBlockOptionLoc *loc)
> +{
> +    broker->err_ret = 0;
> +
> +    switch (loc->prot_type) {
> +    case QB_PROTO_FILE:
> +        if (loc->prot_op.o_file.filename == NULL) {
> +            set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                           "Filename was not set.");
> +            goto out;
> +        }
> +        if (path_has_protocol(loc->prot_op.o_file.filename) > 0) {
> +            set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                           "filename [%s] had protocol.",
> +                           loc->prot_op.o_file.filename);
> +            goto out;
> +        }
> +        break;
> +    default:
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                       "Protocol type [%d] was not valid.",
> +                       loc->prot_type);
> +        break;
> +    }
> +
> + out:
> +    return broker->err_ret;
> +}
> +
> +/* translate loc structure to internal filename, returned char* need free,
> + * assuming filename is not NULL. *filename would be set to NULL if no valid
> + * filename found. *filename must be freed later.
> + * return 0 if no error with *filename set.
> + */
> +static int loc2filename(struct QBroker *broker,
> +                        struct QBlockOptionLoc *loc,
> +                        char **filename)
> +{
> +    broker->err_ret = 0;
> +
> +    if (*filename != NULL) {
> +        CLEAN_FREE(*filename);
> +    }
> +    switch (loc->prot_type) {
> +    case QB_PROTO_FILE:
> +        *filename = strdup(loc->prot_op.o_file.filename);
> +        if (*filename == NULL) {
> +            set_broker_err_nomem(broker);
> +        }
> +        break;
> +    default:
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                 "protocol type [%d] is not supported.",
> +                  loc->prot_type);
> +        break;
> +    }
> +
> +    return broker->err_ret;
> +}
> +
> +/* translate filename to location, loc->prot_type = NONE if fail, filename
> +   must be valid. loc internal char pointer must be freed later.
> + * return 0 if no error.
> + */
> +static int filename2loc(struct QBroker *broker,
> +                        struct QBlockOptionLoc *loc,
> +                        const char *filename)
> +{
> +    broker->err_ret = 0;
> +
> +    if (path_has_protocol(filename) > 0) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                     "Filename [%s] had protocol, not supportted now.",

supported

> +                     filename);
> +        goto out;
> +    }
> +
> +    loc->prot_type = QB_PROTO_FILE;
> +    switch (loc->prot_type) {
> +    case QB_PROTO_FILE:
> +        loc->prot_op.o_file.filename = strdup(filename);
> +        if (loc->prot_op.o_file.filename == NULL) {
> +            set_broker_err_nomem(broker);
> +            goto out;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +
> + out:
> +    return broker->err_ret;
> +}
> +
> +/* return 0 if OK, or qblock error number */
> +static int set_backing_file_options(struct QBroker *broker,
> +                                    QEMUOptionParameter *param,
> +                                    struct QBlockOptionLoc *loc,
> +                                    enum QBlockFormat *fmt)
> +{
> +    char *backing_filename = NULL;
> +    const char *fmtstr_backing = NULL;
> +    int ret = 0;
> +
> +    if (loc == NULL) {
> +        goto out;
> +    }
> +
> +    ret = loc2filename(broker, loc, &backing_filename);
> +    /* ret can < 0 if loc have not been set, mean user did not specify backing
> +       file */
> +    if (ret == QB_ERR_MEM_ERR) {
> +        goto out;
> +    }
> +    ret = 0;
> +
> +    if (backing_filename) {
> +        ret = set_option_parameter(param,
> +                            BLOCK_OPT_BACKING_FILE, backing_filename);
> +        assert(ret == 0);
> +        if (fmt == NULL) {
> +            goto out;
> +        }
> +        fmtstr_backing = fmt2str(*fmt);
> +        if (fmtstr_backing) {
> +            ret = set_option_parameter(param,
> +                                BLOCK_OPT_BACKING_FMT, fmtstr_backing);
> +            assert(ret == 0);
> +        }
> +    }
> +
> + out:
> +    FUNC_FREE(backing_filename);
> +    return ret;
> +}
> +
> +int qb_create(struct QBroker *broker,
> +              struct QBlockState *qbs,
> +              struct QBlockOptionLoc *loc,
> +              struct QBlockOptionFormat *fmt,
> +              int flag)
> +{
> +    int ret = 0, bd_ret;
> +    char *filename = NULL;
> +    BlockDriverState *bs = NULL;
> +    BlockDriver *drv = NULL, *backing_drv = NULL;
> +    bool tmp_bool;
> +
> +    const char *fmtstr = NULL, *tmp = NULL;
> +    QEMUOptionParameter *param = NULL, *create_options = NULL;
> +    QEMUOptionParameter *backing_fmt, *backing_file, *size;
> +    struct QBlockOption_fmt_cow *o_cow = NULL;
> +    struct QBlockOption_fmt_qed *o_qed = NULL;
> +    struct QBlockOption_fmt_qcow *o_qcow = NULL;
> +    struct QBlockOption_fmt_qcow2 *o_qcow2 = NULL;
> +    struct QBlockOption_fmt_raw *o_raw = NULL;
> +    struct QBlockOption_fmt_rbd *o_rbd = NULL;
> +    struct QBlockOption_fmt_sheepdog *o_sd = NULL;
> +    struct QBlockOption_fmt_vdi *o_vdi = NULL;
> +    struct QBlockOption_fmt_vmdk *o_vmdk = NULL;
> +    struct QBlockOption_fmt_vpc *o_vpc = NULL;
> +
> +
> +    /* check parameters */
> +    if (flag & (~LIBQBLOCK_O_VALID_MASK)) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                           "invalid flag was set.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    if ((loc == NULL) || (qbs == NULL) || (fmt == NULL)) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                          "Got unexpected NULL pointer in parameters.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    ret = loc_check_params(broker, loc);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    /* internal translate */
> +    ret = loc2filename(broker, loc, &filename);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    fmtstr = fmt2str(fmt->fmt_type);
> +    if (fmtstr == NULL) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                 "Got unexpected NULL pointer in parameters.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    drv = bdrv_find_format(fmtstr);
> +    assert(drv != NULL);
> +
> +    create_options = append_option_parameters(create_options,
> +                                              drv->create_options);
> +    param = parse_option_parameters("", create_options, param);
> +
> +    switch (fmt->fmt_type) {
> +    case QB_FMT_COW:
> +        o_cow = &(fmt->fmt_op.o_cow);
> +        bd_ret = set_option_parameter_int(param,
> +                                BLOCK_OPT_SIZE, o_cow->virt_size);
> +        assert(bd_ret == 0);
> +        /* do not need to check loc, it may be not set */
> +        ret = set_backing_file_options(broker, param,
> +                                       &o_cow->backing_loc, NULL);
> +        if (ret != 0) {
> +            goto out;
> +        }
> +        break;
> +    case QB_FMT_QED:
> +        o_qed = &(fmt->fmt_op.o_qed);
> +        bd_ret = set_option_parameter_int(param,
> +                                BLOCK_OPT_SIZE, o_qed->virt_size);
> +        assert(bd_ret == 0);
> +        ret = set_backing_file_options(broker, param,
> +                                 &o_qed->backing_loc, &o_qed->backing_fmt);
> +        if (ret != 0) {
> +            goto out;
> +        }
> +        bd_ret = set_option_parameter_int(param,
> +                                BLOCK_OPT_CLUSTER_SIZE, o_qed->cluster_size);
> +        assert(bd_ret == 0);
> +        bd_ret = set_option_parameter_int(param,
> +                                BLOCK_OPT_TABLE_SIZE, o_qed->table_size);
> +        assert(bd_ret == 0);
> +        break;
> +    case QB_FMT_QCOW:
> +        o_qcow = &(fmt->fmt_op.o_qcow);
> +        bd_ret = set_option_parameter_int(param,
> +                                BLOCK_OPT_SIZE, o_qcow->virt_size);
> +        assert(bd_ret == 0);
> +        ret = set_backing_file_options(broker, param,
> +                                       &o_qcow->backing_loc, NULL);
> +        if (ret != 0) {
> +            goto out;
> +        }
> +        tmp = o_qcow->encrypt ? "on" : "off";
> +        bd_ret = set_option_parameter(param, BLOCK_OPT_ENCRYPT, tmp);
> +        assert(bd_ret == 0);
> +        break;
> +    case QB_FMT_QCOW2:
> +        o_qcow2 = &(fmt->fmt_op.o_qcow2);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_SIZE, o_qcow2->virt_size);
> +        assert(bd_ret == 0);
> +        ret = set_backing_file_options(broker, param,
> +                              &o_qcow2->backing_loc, &o_qcow2->backing_fmt);
> +        if (ret != 0) {
> +            goto out;
> +        }
> +        tmp = o_qcow2->encrypt ? "on" : "off";
> +        bd_ret = set_option_parameter(param, BLOCK_OPT_ENCRYPT, tmp);
> +        assert(bd_ret == 0);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_CLUSTER_SIZE, o_qcow2->cluster_size);
> +        assert(bd_ret == 0);
> +
> +        if (o_qcow2->cpt_lv != QBO_FMT_QCOW2_CPT_NONE) {
> +            tmp = o_qcow2->cpt_lv == QBO_FMT_QCOW2_CPT_V010 ? "0.10" : "1.1";
> +            bd_ret = set_option_parameter(param,
> +                              BLOCK_OPT_COMPAT_LEVEL, tmp);
> +            assert(bd_ret == 0);
> +        }
> +
> +        if (o_qcow2->pre_mode != QBO_FMT_QCOW2_PREALLOC_NONE) {
> +            tmp = o_qcow2->pre_mode == QBO_FMT_QCOW2_PREALLOC_OFF ?
> +                                         "off" : "metadata";
> +            bd_ret = set_option_parameter(param,
> +                              BLOCK_OPT_PREALLOC, tmp);
> +            assert(bd_ret == 0);
> +        }
> +        break;
> +
> +    case QB_FMT_RAW:
> +        o_raw = &(fmt->fmt_op.o_raw);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_SIZE, o_raw->virt_size);
> +        assert(bd_ret == 0);
> +        break;
> +    case QB_FMT_RBD:
> +        o_rbd = &(fmt->fmt_op.o_rbd);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_SIZE, o_rbd->virt_size);
> +        assert(bd_ret == 0);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_CLUSTER_SIZE, o_rbd->cluster_size);
> +        assert(bd_ret == 0);
> +        break;
> +    case QB_FMT_SHEEPDOG:
> +        o_sd = &(fmt->fmt_op.o_sheepdog);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_SIZE, o_sd->virt_size);
> +        assert(bd_ret == 0);
> +        ret = set_backing_file_options(broker, param,
> +                                       &o_sd->backing_loc, NULL);
> +        if (ret != 0) {
> +            goto out;
> +        }
> +        if (o_sd->pre_mode != QBO_FMT_SD_PREALLOC_NONE) {
> +            tmp = o_sd->pre_mode == QBO_FMT_SD_PREALLOC_OFF ? "off" : "full";
> +            bd_ret = set_option_parameter(param,
> +                              BLOCK_OPT_PREALLOC, tmp);
> +            assert(bd_ret == 0);
> +        }
> +        break;
> +    case QB_FMT_VDI:
> +        o_vdi = &(fmt->fmt_op.o_vdi);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_SIZE, o_vdi->virt_size);
> +        assert(bd_ret == 0);
> +        /* following option is not always valid depends on configuration */
> +        set_option_parameter_int(param,
> +                              BLOCK_OPT_CLUSTER_SIZE, o_vdi->cluster_size);
> +        if (o_vdi->pre_mode != QBO_FMT_VDI_PREALLOC_NONE) {
> +            tmp_bool = o_sd->pre_mode == QBO_FMT_VDI_PREALLOC_TRUE ?
> +                                                     true : false;
> +            set_option_parameter_int(param, "static", tmp_bool);
> +        }
> +        break;
> +    case QB_FMT_VMDK:
> +        o_vmdk = &(fmt->fmt_op.o_vmdk);
> +        bd_ret = set_option_parameter_int(param,
> +                              BLOCK_OPT_SIZE, o_vmdk->virt_size);
> +        assert(bd_ret == 0);
> +        ret = set_backing_file_options(broker, param,
> +                                       &o_vmdk->backing_loc, NULL);
> +        if (ret != 0) {
> +            goto out;
> +        }
> +
> +        if (o_vmdk->cpt_lv != QBO_FMT_VMDK_CPT_NONE) {
> +            tmp_bool = o_vmdk->cpt_lv == QBO_FMT_VMDK_CPT_VMDKV6_TRUE ?
> +                                                     true : false;
> +            bd_ret = set_option_parameter_int(param, BLOCK_OPT_COMPAT6,
> +                                                     tmp_bool);
> +            assert(bd_ret == 0);
> +        }
> +        if (o_vmdk->subfmt != QBO_FMT_VMDK_SUBFMT_MONOLITHIC_NONE) {
> +            switch (o_vmdk->subfmt) {
> +            case QBO_FMT_VMDK_SUBFMT_MONOLITHIC_SPARSE:
> +                tmp = "monolithicSparse";
> +                break;
> +            case QBO_FMT_VMDK_SUBFMT_MONOLITHIC_FLAT:
> +                tmp = "monolithicFlat";
> +                break;
> +            case QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_SPARSE:
> +                tmp = "twoGbMaxExtentSparse";
> +                break;
> +            case QBO_FMT_VMDK_SUBFMT_TWOGBMAX_EXTENT_FLAT:
> +                tmp = "twoGbMaxExtentFlat";
> +                break;
> +            case QBO_FMT_VMDK_SUBFMT_STREAM_OPTIMIZED:
> +                tmp = "streamOptimized";
> +                break;
> +            default:
> +                assert(false);
> +                break;
> +            }
> +            bd_ret = set_option_parameter(param,
> +                              BLOCK_OPT_SUBFMT, tmp);
> +            assert(bd_ret == 0);
> +        }
> +        break;
> +    case QB_FMT_VPC:
> +        o_vpc = &(fmt->fmt_op.o_vpc);
> +        bd_ret = set_option_parameter_int(param,
> +                               BLOCK_OPT_SIZE, o_vpc->virt_size);
> +        assert(bd_ret == 0);
> +        if (o_vpc->subfmt != QBO_FMT_VPC_SUBFMT_NONE) {
> +            tmp = o_vpc->subfmt == QBO_FMT_VPC_SUBFMT_DYNAMIC ?
> +                                               "dynamic" : "fixed";
> +            bd_ret = set_option_parameter(param,
> +                               BLOCK_OPT_SUBFMT, tmp);
> +            assert(bd_ret == 0);
> +        }
> +        break;
> +    default:
> +        assert(false);
> +        break;
> +    }
> +
> +    backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> +    if (backing_file && backing_file->value.s) {
> +        if (!strcmp(filename, backing_file->value.s)) {
> +            set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                          "Backing file is the same with new file.");
> +            ret = broker->err_ret;
> +            goto out;
> +        }
> +    }
> +
> +    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
> +    if (backing_fmt && backing_fmt->value.s) {
> +        backing_drv = bdrv_find_format(backing_fmt->value.s);
> +        assert(backing_drv != NULL);
> +    }
> +
> +    size = get_option_parameter(param, BLOCK_OPT_SIZE);
> +    if (size && size->value.n <= 0) {
> +        if (backing_file && backing_file->value.s) {
> +            uint64_t size;
> +            char buf[32];
> +            int back_flags;
> +
> +            /* backing files always opened read-only */
> +            back_flags =
> +                flag &
> +                ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
> +
> +            bs = bdrv_new("");
> +
> +            ret = bdrv_open(bs, backing_file->value.s,
> +                                back_flags, backing_drv);
> +            if (ret < 0) {
> +                set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                               "Failed to open the backing file.");
> +                ret = broker->err_ret;
> +                goto out;
> +            }
> +            bdrv_get_geometry(bs, &size);
> +            size *= SECTOR_SIZE;
> +
> +            snprintf(buf, sizeof(buf), "%" PRId64, size);
> +            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> +        } else {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                           "Neither size or backing file was not set.");
> +            ret = broker->err_ret;
> +            goto out;
> +        }
> +    }
> +
> +    bd_ret = bdrv_create(drv, filename, param);
> +
> +
> +    if (bd_ret < 0) {
> +        const char *errstr;
> +        if (bd_ret == -ENOTSUP) {
> +            errstr = "formatting option not supported.";
> +        } else if (bd_ret == -EFBIG) {
> +            errstr = "The image size is too large.";
> +        } else {
> +            errstr = "Error in creating the image.";
> +        }
> +        set_broker_err(broker, QB_ERR_INTERNAL_ERR, errstr);
> +        ret = broker->err_ret;
> +    }
> +
> +out:
> +    free_option_parameters(create_options);
> +    free_option_parameters(param);
> +    FUNC_FREE(filename);
> +    if (bs) {
> +        bdrv_delete(bs);
> +    }
> +
> +    return ret;
> +}
> +
> +int qb_open(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            struct QBlockOptionLoc *loc,
> +            struct QBlockOptionFormat *fmt,
> +            int flag)
> +{
> +    int ret = 0, bd_ret;
> +    BlockDriverState *bs;
> +    BlockDriver *bd;
> +    const char *fmtstr;
> +    char *filename = NULL;
> +
> +    /* take care of user settings */
> +    /* do nothing now */
> +
> +    /* check parameters */
> +    if (flag & (~LIBQBLOCK_O_VALID_MASK)) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                      "Invalid flag was set.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    if ((loc == NULL) || (qbs == NULL)) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                      "Got unexpected NULL pointer in parameters.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    ret = loc_check_params(broker, loc);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    /* internal translate */
> +    ret = loc2filename(broker, loc, &filename);
> +    if (ret != 0) {
> +        goto out;
> +    }
> +
> +    fmtstr = NULL;
> +    bd = NULL;
> +    if (fmt != NULL) {
> +        fmtstr = fmt2str(fmt->fmt_type);
> +    }
> +
> +    if (fmtstr != NULL) {
> +        bd = bdrv_find_format(fmtstr);
> +        assert(bd != NULL);
> +    }
> +
> +    /* do real openning */
> +    bs = qbs->bdrvs;
> +    bd_ret = bdrv_open(bs, filename, flag, bd);
> +    if (bd_ret < 0) {
> +        set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                      "Failed in opening with driver, bd_ret is %d.", bd_ret);
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    if (qbs->filename != NULL) {
> +        FUNC_FREE(qbs->filename);
> +    }
> +    qbs->filename = strdup(filename);
> +    if (qbs->filename == NULL) {
> +        set_broker_err_nomem(broker);
> +        ret = broker->err_ret;
> +        bdrv_close(bs);
> +        goto out;
> +    }
> +
> + out:
> +    FUNC_FREE(filename);
> +    return ret;
> +}
> +
> +void qb_close(struct QBroker *broker,
> +              struct QBlockState *qbs)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = qbs->bdrvs;
> +
> +    if (qbs->filename != NULL) {
> +        CLEAN_FREE(qbs->filename);
> +        bdrv_close(bs);
> +    }
> +    return;
> +}
> +
> +int qb_read(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            const void *buf,

We write to 'buf', it can't be const.

> +            size_t len,
> +            off_t offset)
> +{
> +    int bd_ret;
> +    BlockDriverState *bs;
> +
> +    broker->err_ret = 0;
> +    bs = qbs->bdrvs;
> +
> +    assert((len & SECTOR_SIZE_MASK) == 0);
> +    assert((offset & SECTOR_SIZE_MASK) == 0);
> +    bd_ret = bdrv_read(bs, offset >> SECTOR_SIZE_BITS_NUM,
> +                       (uint8_t *)buf, len >> SECTOR_SIZE_BITS_NUM);

With 'const' removed, I think the cast can be omitted.

> +    if (bd_ret < 0) {
> +        if (bd_ret == -EIO) {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                           "Generic I/O error.");
> +        } else if (bd_ret == -ENOMEDIUM) {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                           "No meida inserted.");
> +        } else if (bd_ret == -EINVAL) {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                          "Sector was not correct.");
> +        } else {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                          "Internal error.");
> +        }
> +    }
> +    return broker->err_ret;
> +}
> +
> +int qb_write(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            const void *buf,
> +            size_t len,
> +            off_t offset)
> +{
> +    int bd_ret;
> +    BlockDriverState *bs;
> +
> +    broker->err_ret = 0;
> +    bs = qbs->bdrvs;
> +
> +    assert((len & SECTOR_SIZE_MASK) == 0);
> +    assert((offset & SECTOR_SIZE_MASK) == 0);
> +    bd_ret = bdrv_write(bs, offset >> SECTOR_SIZE_BITS_NUM,
> +                       (uint8_t *)buf, len >> SECTOR_SIZE_BITS_NUM);

Also this cast seems useless.

> +    if (bd_ret < 0) {
> +        if (bd_ret == -EIO) {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                           "Generic I/O error.");
> +        } else if (bd_ret == -ENOMEDIUM) {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                           "No meida inserted.");
> +        } else if (bd_ret == -EINVAL) {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                          "Sector was not correct.");
> +        } else {
> +            set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                          "Internal error.");
> +        }
> +    }
> +    return broker->err_ret;
> +}
> +
> +int qb_flush(struct QBroker *broker,
> +             struct QBlockState *qbs)
> +{
> +    int bd_ret;
> +    BlockDriverState *bs;
> +
> +    broker->err_ret = 0;
> +    bs = qbs->bdrvs;
> +    bd_ret = bdrv_flush(bs);
> +    if (bd_ret < 0) {
> +        set_broker_err(broker, QB_ERR_INTERNAL_ERR,
> +                       "Internal error.");
> +    }
> +    return broker->err_ret;
> +}
> +
> +int qb_is_allocated(struct QBroker *broker,
> +                    struct QBlockState *qbs,
> +                    int64_t sector_num,
> +                    int nb_sectors,
> +                    int *pnum)
> +{
> +    int ret;
> +    BlockDriverState *bs;
> +
> +    broker->err_ret = 0;
> +    bs = qbs->bdrvs;
> +
> +    if (qbs->filename == NULL) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                       "Image was not opened first.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    ret = bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
> +
> + out:
> +    return ret;
> +}
> +
> +int qb_info_image_static_get(struct QBroker *broker,
> +                             struct QBlockState *qbs,
> +                             struct QBlockInfoImageStatic **info)
> +{
> +    int ret = 0;
> +    BlockDriverState *bs;
> +    struct QBlockInfoImageStatic *info_tmp;
> +    const char *fmt_str;
> +    size_t total_sectors;
> +    char backing_filename[1024];
> +
> +    if (qbs->filename == NULL) {
> +        set_broker_err(broker, QB_ERR_INVALID_PARAM,
> +                       "Block Image was not openned.");
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    info_tmp = FUNC_CALLOC(1, sizeof(struct QBlockInfoImageStatic));
> +    if (info_tmp == NULL) {
> +        set_broker_err_nomem(broker);
> +        ret = broker->err_ret;
> +        goto out;
> +    }
> +
> +    bs = qbs->bdrvs;
> +
> +    ret = filename2loc(broker,
> +                       &(info_tmp->loc),
> +                       qbs->filename);
> +    if (ret < 0) {
> +        goto free;
> +    }
> +
> +    fmt_str = bdrv_get_format_name(bs);
> +    info_tmp->fmt_type = str2fmt(fmt_str);
> +
> +    bdrv_get_geometry(bs, &total_sectors);
> +    info_tmp->virt_size = total_sectors * SECTOR_SIZE;
> +
> +    info_tmp->allocated_size = bdrv_get_allocated_file_size(bs);
> +    info_tmp->encrypt = bdrv_is_encrypted(bs);
> +
> +    bdrv_get_full_backing_filename(bs, backing_filename,
> +                                   sizeof(backing_filename));
> +    if (backing_filename[0] != '\0') {
> +        ret = filename2loc(broker,
> +                           &(info_tmp->backing_loc),
> +                           backing_filename);
> +        if (ret < 0) {
> +            goto free;
> +        }
> +    }
> +
> +    *info = info_tmp;
> +
> + out:
> +    return ret;
> + free:
> +    qb_info_image_static_delete(broker, &info_tmp);
> +    return ret;
> +}
> +
> +/* free locations if it has string allocated on heap. */
> +static void loc_free(struct QBlockOptionLoc *loc)
> +{
> +    switch (loc->prot_type) {
> +    case QB_PROTO_FILE:
> +        CLEAN_FREE(loc->prot_op.o_file.filename);
> +        break;
> +    default:
> +        break;
> +    }
> +}
> +
> +void qb_info_image_static_delete(struct QBroker *broker,
> +                                 struct QBlockInfoImageStatic **info)
> +{
> +    loc_free(&(*info)->loc);
> +    loc_free(&(*info)->backing_loc);
> +
> +    CLEAN_FREE(*info);
> +}
> diff --git a/libqblock/libqblock.h b/libqblock/libqblock.h
> new file mode 100644
> index 0000000..e64e1fd
> --- /dev/null
> +++ b/libqblock/libqblock.h
> @@ -0,0 +1,251 @@
> +/*
> + * QEMU block layer library
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Wenchao Xia   <xiawenc@linux.vnet.ibm.com>
> + *
> + * 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_H
> +#define LIBQBLOCK_H
> +
> +#include "libqblock-types.h"
> +#include "libqblock-error.h"
> +
> +
> +/**
> + * libqblock_init: Initialize the library
> + */
> +void libqblock_init(void);
> +
> +/**
> + * qb_broker_new: allocate a new broker
> + *
> + * Broker is used to pass operation to libqblock, and got feed back from it.
> + *
> + * Returns 0 on success, negative value on fail.
> + *
> + * @broker: used to receive the created struct.
> + */
> +int qb_broker_new(struct QBroker **broker);
> +
> +/**
> + * qb_broker_delete: delete broker
> + *
> + * Broker will be freed and set to NULL.
> + *
> + * @broker: operation broker to be deleted.
> + */
> +void qb_broker_delete(struct QBroker **broker);
> +
> +/**
> + * qb_state_new: allocate a new QBloctState struct
> + *
> + * Following qblock action were based on this struct
> + *
> + * Returns 0 if succeed, negative value on fail.
> + *
> + * @broker: operation broker.
> + * @qbs: used to receive the created struct.
> + */
> +int qb_state_new(struct QBroker *broker,
> +                 struct QBlockState **qbs);
> +
> +/**
> + * qb_state_delete: free a QBloctState struct
> + *
> + * if it is opened, a qb_close must be called before free.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to the struct's pointer.
> + */
> +void qb_state_delete(struct QBroker *broker,
> +                     struct QBlockState **qbs);
> +
> +/**
> + * qb_ol_new: create a new struct QBlockOptionLoc.
> + *
> + * return 0 on success, negative on fail.
> + *
> + * @broker: operation broker.
> + * @op: pointer to receive the new created one.
> + */
> +int qb_ol_new(struct QBroker *broker,
> +              struct QBlockOptionLoc **op);
> +
> +/**
> + * qb_ol_delete: free a struct QBlockOptionLoc.
> + *
> + * @broker: operation broker.
> + * @op: pointer to the object, *op would be set to NULL.
> + */
> +void qb_ol_delete(struct QBroker *broker,
> +                  struct QBlockOptionLoc **op);
> +
> +/**
> + * qb_of_new: create a new QBlockOptionFormat structure.
> + *
> + * return 0 on success, negative on fail.
> + *
> + * @broker: operation broker.
> + * @op: pointer that will receive created struct.
> + */
> +int qb_of_new(struct QBroker *broker,
> +              struct QBlockOptionFormat **op);
> +
> +/**
> + * qb_of_delete: free QBlockOptionFormat structure.
> + *
> + * @broker: operation broker.
> + * @op: pointer to the struct, *op would be set to NULL.
> + */
> +void qb_of_delete(struct QBroker *broker,
> +                  struct QBlockOptionFormat **op);
> +
> +
> +/**
> + * qb_open: open a block object.
> + *
> + * return 0 on success, negative on fail.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @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 NULL if you want auto discovery the format.
> + * @flag: behavior control flags.
> + */
> +int qb_open(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            struct QBlockOptionLoc *loc,
> +            struct QBlockOptionFormat *fmt,
> +            int flag);
> +
> +/**
> + * qb_close: close a block object.
> + *
> + * qb_flush is automaticlly done inside.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + */
> +void qb_close(struct QBroker *broker,
> +              struct QBlockState *qbs);
> +
> +/**
> + * qb_create: create a block image or object.
> + *
> + * Note: Create operation would not open the image automatically.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @loc: location options for open, how to find the image.
> + * @fmt: format options, how to extract the data.
> + * @flag: behavior control flags.
> + */
> +int qb_create(struct QBroker *broker,
> +              struct QBlockState *qbs,
> +              struct QBlockOptionLoc *loc,
> +              struct QBlockOptionFormat *fmt,
> +              int flag);
> +
> +
> +/* sync access */
> +/**
> + * qb_read: block sync read.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @buf: buffer that receive the content.
> + * @len: length to read.
> + * @offset: offset in the block data.
> + */
> +int qb_read(struct QBroker *broker,
> +            struct QBlockState *qbs,
> +            const void *buf,
> +            size_t len,
> +            off_t offset);
> +/**
> + * qb_write: block sync write.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @buf: buffer that receive the content.
> + * @len: length to write.
> + * @offset: offset in the block data.
> + */
> +int qb_write(struct QBroker *broker,
> +             struct QBlockState *qbs,
> +             const void *buf,
> +             size_t len,
> +             off_t offset);
> +
> +/**
> + * qb_flush: block sync flush.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + */
> +int qb_flush(struct QBroker *broker,
> +             struct QBlockState *qbs);
> +
> +
> +/* advance image APIs */
> +/**
> + * qb_is_allocated: check if following sectors was allocated on the image.
> + *
> + * return negative on fail, 0 or 1 on success. 0 means unallocated, 1 means
> + *allocated.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @sector_num: start sector number. If 'sector_num' is beyond the end of the
> + *disk image the return value is 0 and 'pnum' is set to 0.
> + * @nb_sectors: how many sector would be checked, it is the max value 'pnum'
> + *should be set to.  If nb_sectors goes beyond the end of the disk image it
> + *will be clamped.
> + * @pnum: pointer to receive how many sectors are allocated or unallocated.
> + */
> +int qb_is_allocated(struct QBroker *broker,
> +                    struct QBlockState *qbs,
> +                    int64_t sector_num,
> +                    int nb_sectors,
> +                    int *pnum);
> +
> +/* image information */
> +/**
> + * qb_get_image_info: get image info.
> + *
> + * return negative on fail, 0 on success.
> + *
> + * @broker: operation broker.
> + * @qbs: pointer to struct QBlockState.
> + * @info: pointer that would receive the information.
> + */
> +int qb_info_image_static_get(struct QBroker *broker,
> +                             struct QBlockState *qbs,
> +                             struct QBlockInfoImageStatic **info);
> +
> +/**
> + * qb_delete_image_info: free image info.
> + *
> + * @broker: operation broker.
> + * @info: pointer to the information struct.
> + */
> +void qb_info_image_static_delete(struct QBroker *broker,
> +                                 struct QBlockInfoImageStatic **info);
> +
> +#endif
> --
> 1.7.1
>
>
>

于 2012-9-3 21:18, Paolo Bonzini 写道:
> Il 03/09/2012 11:18, Wenchao Xia ha scritto:
>>    1 QBroker. These structure was used to retrieve errors, every thread must
>> create one first, Later maybe thread related staff could be added into it.
>
> Can you use GError instead?
>
   read through the GError doc, GError is defined as following:
struct GError {
   GQuark       domain;
   gint         code;
   gchar       *message;
};
   I am worried about the message member, I guess program would be
aborted if OOM, which I was tring to avoid, so I used char err_msg[1024]
in my code, and make things simpler.

>>    3 QBlockInfoImageStatic. Now it is not folded with location and format.
>
> What does "Static" mean?
>
  It is about sorting the information into following kinds:
1) static. It is values that defined at creating time/modifying time,
mostly some settings, and it would not be automatically changed in I/O.
2) dynamic. Some information that would changes in I/O and other
operations, such as allocated_size, snapshots.
3) statistics.
   Now only static one is provided, so I added _static suffix.

> Paolo
>

Thank u for the careful reviewing of my codes, I will write down
the typo errors you mentioned on a note.

> On 09/03/2012 03:18 AM, Wenchao Xia wrote:
>>    This patch contains the major APIs in the library.
>> Important APIs:
>>    1 QBroker. These structure was used to retrieve errors, every thread must
>> create one first, Later maybe thread related staff could be added into it.
>>    2 QBlockState. It stands for an block image object.
>>    3 QBlockInfoImageStatic. Now it is not folded with location and format.
>>    4 ABI was kept with reserved members.
>>
>> structure, because it would cause caller more codes to find one member.
>
> Commit message snafu?
>
  a wrong paste, sorry.

>> +/**
>> + * libqblock_init: Initialize the library
>> + */
>> +void libqblock_init(void);
>
> Is this function safe to call more than once?  Even tighter, is it safe
> to call this function simultaneously from multiple threads?
>
   No, it should be only called once, any other thread should not call
it again, will document it. About the multiple thread user case, qemu
block layer can't support that now, will fix that later.

>> +
>> +/**
>> + * qb_broker_new: allocate a new broker
>> + *
>> + * Broker is used to pass operation to libqblock, and got feed back from it.
>
> s/got feed back/get feedback/
>
>> + *
>> + * Returns 0 on success, negative value on fail.
>
> Is there any particular interpretation to this negative value, such as
> negative errno constant, or always -1?
>
   Yes, negative values are defined with macros in the header file.

>> +
>> +/**
>> + * qb_state_new: allocate a new QBloctState struct
>
> s/Bloct/Block/
>
>> + *
>> + * Following qblock action were based on this struct
>
> Didn't read well.  Did you mean:
>
> Subsequent qblock actions will use this struct
>
   Yes.

>> + *
>> + * Returns 0 if succeed, negative value on fail.
>
> Again, is there any particular meaning to which negative value is returned?
>
>> +
>> +/**
>> + * qb_open: open a block object.
>> + *
>> + * return 0 on success, negative on fail.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @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 NULL if you want auto discovery the format.
>
> set to NULL if you want to auto-discover the format
>
> Maybe also add a warning about the inherent security risks of attempting
> format auto-discovery (any raw image must NOT be probed, as the raw
> image can emulate any other format and cause qemu to chase down chains
> where it should not).
>
   it seems qemu-img could find out that an image is raw correctly by
probing, do you mean give a warning saying that this image is probably
some formats that qemu do not supported, such as virtual box's image?

>> + * @flag: behavior control flags.
>
> What flags are currently defined?
>
   It is the flags defined in the header file, such as LIBQBLOCK_O_RDWR,
will document it.

>> + */
>> +int qb_open(struct QBroker *broker,
>> +            struct QBlockState *qbs,
>> +            struct QBlockOptionLoc *loc,
>> +            struct QBlockOptionFormat *fmt,
>> +            int flag);
>> +
>> +/**
>> + * qb_close: close a block object.
>> + *
>> + * qb_flush is automaticlly done inside.
>
> s/automaticlly/automatically/
>
>> +/**
>> + * qb_create: create a block image or object.
>> + *
>> + * Note: Create operation would not open the image automatically.
>> + *
>> + * return negative on fail, 0 on success.
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @loc: location options for open, how to find the image.
>> + * @fmt: format options, how to extract the data.
>> + * @flag: behavior control flags.
>
> Again, what flags are defined?
>
>> +
>> +/* sync access */
>> +/**
>> + * qb_read: block sync read.
>> + *
>> + * return negative on fail, 0 on success.
>
> Shouldn't this instead return the number of successfully read bytes, to
> allow for short reads if offset exceeds end-of-file?  If so, should it
> return ssize_t instead of int?
>
   I had this idea before too, let'me check if qemu block layer can
provide this functionality,

>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @buf: buffer that receive the content.
>
> s/receive/receives/
>
>> + * @len: length to read.
>
> Is there a magic length for reading the entire file in one go?
>
   no, if so where should I put with the result.

>> + * @offset: offset in the block data.
>> + */
>> +int qb_read(struct QBroker *broker,
>> +            struct QBlockState *qbs,
>> +            const void *buf,
>> +            size_t len,
>> +            off_t offset);
>
> You do realize that size_t and off_t are not necessarily the same width;
> but I'm okay with limiting to size_t reads.
>
>> +/**
>> + * qb_write: block sync write.
>> + *
>> + * return negative on fail, 0 on success.
>
> Again, this should probably return number of successfully written bytes,
> as an ssize_t.
>
>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @buf: buffer that receive the content.
>
> s/receive/supplies/
>
>> +/* advance image APIs */
>> +/**
>> + * qb_is_allocated: check if following sectors was allocated on the image.
>> + *
>> + * return negative on fail, 0 or 1 on success. 0 means unallocated, 1 means
>> + *allocated.
>
> Formatting is off.
>
   coming later.

>> + *
>> + * @broker: operation broker.
>> + * @qbs: pointer to struct QBlockState.
>> + * @sector_num: start sector number. If 'sector_num' is beyond the end of the
>> + *disk image the return value is 0 and 'pnum' is set to 0.
>> + * @nb_sectors: how many sector would be checked, it is the max value 'pnum'
>> + *should be set to.  If nb_sectors goes beyond the end of the disk image it
>> + *will be clamped.
>> + * @pnum: pointer to receive how many sectors are allocated or unallocated.
>
> This interface requires the user to know how big a sector is.  Would it
> be any more convenient to the user to pass offsets, rather than sector
> numbers; and/or have a function for converting between offsets and
> sector numbers?
>
   OK, I need a function returning how big a sector is in an image.

>> + */
>> +int qb_is_allocated(struct QBroker *broker,
>> +                    struct QBlockState *qbs,
>> +                    int64_t sector_num,
>> +                    int nb_sectors,
>
> Shouldn't nb_sectors be size_t?
>
>> +                    int *pnum);
>
> Exactly how does the *pnum argument work?  This interface looks like it
> isn't fully thought out yet.  Either I want to know if a chunk of
> sectors is allocated (I supply start and length of sectors to check),
> regardless of how many sectors beyond that point are also allocated
> (pnum makes no sense); or I want to know how many sectors are allocated
> from a given point (I supply start, and the function returns length, so
> nb_sectors makes no sense).  Either way, I think you are supplying too
> many parameters for how I envision checking for allocated sectors.
>

Il 04/09/2012 05:15, Wenchao Xia ha scritto:
>>
>> Can you use GError instead?
>>
>   read through the GError doc, GError is defined as following:
> struct GError {
>   GQuark       domain;
>   gint         code;
>   gchar       *message;
> };
>   I am worried about the message member, I guess program would be
> aborted if OOM, which I was tring to avoid, so I used char err_msg[1024]
> in my code, and make things simpler.

That's true.  On the other hand, and IMHO, not aborting in the library
code is a non-goal as long as the rest of the block layer still does.

>>>    3 QBlockInfoImageStatic. Now it is not folded with location and
>>> format.
>>
>> What does "Static" mean?
>>
>  It is about sorting the information into following kinds:
> 1) static. It is values that defined at creating time/modifying time,
> mostly some settings, and it would not be automatically changed in I/O.
> 2) dynamic. Some information that would changes in I/O and other
> operations, such as allocated_size, snapshots.
> 3) statistics.
>   Now only static one is provided, so I added _static suffix.

Makes sense, thanks for the clarification.  Perhaps QBlockStaticInfo is
a shorter and simpler name?

Paolo

于 2012-9-3 22:05, Paolo Bonzini 写道:
> Il 03/09/2012 15:56, Eric Blake ha scritto:
>> Exactly how does the *pnum argument work?  This interface looks like it
>> isn't fully thought out yet.  Either I want to know if a chunk of
>> sectors is allocated (I supply start and length of sectors to check),
>> regardless of how many sectors beyond that point are also allocated
>> (pnum makes no sense);
>
> pnum makes sense if the [start, start+length) range includes both
> allocated and unallocated sectors.
>
>> or I want to know how many sectors are allocated
>> from a given point (I supply start, and the function returns length, so
>> nb_sectors makes no sense).
>
   About using byte offset instead of sector, I think sector is better,
because the allocation status is based on sector, all bytes data in a
sector would have the same status.

> This operation could be O(number of blocks in disk) worst case, so it
> makes sense to provide nb_sectors as an upper bound.  nb_sectors is
> typically dictated by the size of your buffer.
>
> That said, QEMU's internal bdrv_is_allocated function does have one not
> entirely appealing property: the block at start + *pnum might have the
> same state as the block at start + *pnum - 1, even if *pnum < length.
> We may want to work around this in libqblock, but we could also simply
> document it.
>
> Paolo
>
int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
                       int nb_sectors,int *pnum)
will the issue happen when nb_sectors > *pnum? if so it seems a bug,
because caller is asking a range of sectors's allocation status, and
*pnum did not reflect the real status.

>> Either way, I think you are supplying too
>> many parameters for how I envision checking for allocated sectors.
>
   yes, it is a bit confusing, how about:

int qb_check_allocate_status(struct QBroker *broker,
                              struct QBlockState *qbs,
                              offset sector_start,
                              size_t sector_number,
                              size_t *pnum,
                              int *status)
user input sector_start and sector_number to ask check it in this range,
following parameter receive the status, return indicate exception.

>

Il 04/09/2012 09:05, Wenchao Xia ha scritto:
>> That said, QEMU's internal bdrv_is_allocated function does have one not
>> entirely appealing property: the block at start + *pnum might have the
>> same state as the block at start + *pnum - 1, even if *pnum < length.
>> We may want to work around this in libqblock, but we could also simply
>> document it.
>>
>> Paolo
>>
> int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num,
>                       int nb_sectors,int *pnum)
> will the issue happen when nb_sectors > *pnum? if so it seems a bug,
> because caller is asking a range of sectors's allocation status, and
> *pnum did not reflect the real status.

Actually it does.

bdrv_is_allocated says it didn't find out anything about sector start +
*pnum and later.

>>> Either way, I think you are supplying too
>>> many parameters for how I envision checking for allocated sectors.
>>
>   yes, it is a bit confusing, how about:
> 
> int qb_check_allocate_status(struct QBroker *broker,
>                              struct QBlockState *qbs,
>                              offset sector_start,
>                              size_t sector_number,
>                              size_t *pnum,
>                              int *status)
> user input sector_start and sector_number to ask check it in this range,
> following parameter receive the status, return indicate exception.

That's ok too.

But do not use size_t for sectors.  Testing, or reading, or writing 2 TB
with a single call is more than enough.

Paolo

于 2012-9-4 14:50, Paolo Bonzini 写道:
> Il 04/09/2012 05:15, Wenchao Xia ha scritto:
>>>
>>> Can you use GError instead?
>>>
>>    read through the GError doc, GError is defined as following:
>> struct GError {
>>    GQuark       domain;
>>    gint         code;
>>    gchar       *message;
>> };
>>    I am worried about the message member, I guess program would be
>> aborted if OOM, which I was tring to avoid, so I used char err_msg[1024]
>> in my code, and make things simpler.
>
> That's true.  On the other hand, and IMHO, not aborting in the library
> code is a non-goal as long as the rest of the block layer still does.
>
   Hard problem for me, do you have some suggestion about OOM issue?
Using GLib's more functions, such as GError and Gsource main event
loop, would cause this issue more difficult to solve later.
   Do we have an alternative robust lib as glib but reports OOM instead
exit on Linux?

>>>>     3 QBlockInfoImageStatic. Now it is not folded with location and
>>>> format.
>>>
>>> What does "Static" mean?
>>>
>>   It is about sorting the information into following kinds:
>> 1) static. It is values that defined at creating time/modifying time,
>> mostly some settings, and it would not be automatically changed in I/O.
>> 2) dynamic. Some information that would changes in I/O and other
>> operations, such as allocated_size, snapshots.
>> 3) statistics.
>>    Now only static one is provided, so I added _static suffix.
>
> Makes sense, thanks for the clarification.  Perhaps QBlockStaticInfo is
> a shorter and simpler name?
>
   OK.

> Paolo
>

On 09/04/2012 12:42 AM, Wenchao Xia wrote:
>>> +/**
>>> + * libqblock_init: Initialize the library
>>> + */
>>> +void libqblock_init(void);
>>
>> Is this function safe to call more than once?  Even tighter, is it safe
>> to call this function simultaneously from multiple threads?
>>
>   No, it should be only called once, any other thread should not call
> it again, will document it. About the multiple thread user case, qemu
> block layer can't support that now, will fix that later.

What a shame.  That makes libraries much harder to use.  It is much
nicer to design a library where the initialization is idempotent and
thread-safe, to be called from multiple threads.  Consider:

app links against liba and libb;
liba links against libqb
libb links against libqb

How am I supposed to write liba and libb to guarantee only one single
race-free call to libqblock_init, unless libqblock_init() is idempotent?

Also, should there be a counterpart function for tearing down the
resources used by the library when it is no longer needed?  If so, then
that implies reference counting - each call to init atomically increases
the refcount, and the library frees resources only when the refcount
atomically goes back to 0.


>>> + * @fmt: format options, how to extract the data, only valid member
>>> now is
>>> +     fmt->fmt_type, set NULL if you want auto discovery the format.
>>
>> set to NULL if you want to auto-discover the format
>>
>> Maybe also add a warning about the inherent security risks of attempting
>> format auto-discovery (any raw image must NOT be probed, as the raw
>> image can emulate any other format and cause qemu to chase down chains
>> where it should not).
>>
>   it seems qemu-img could find out that an image is raw correctly by
> probing, do you mean give a warning saying that this image is probably
> some formats that qemu do not supported, such as virtual box's image?

No, you got it backwards.  For all non-raw images, qemu can correctly
probe the image.  But for raw images, the guest may have set enough
information in the image to make a probe _think_ that the image is
non-raw, and therefore cause qemu to misbehave.  That is, the security
hole is choosing to probe a raw image, because the probe will not always
successfully return raw.

Il 04/09/2012 13:35, Eric Blake ha scritto:
>> >   No, it should be only called once, any other thread should not call
>> > it again, will document it. About the multiple thread user case, qemu
>> > block layer can't support that now, will fix that later.
> What a shame.  That makes libraries much harder to use.  It is much
> nicer to design a library where the initialization is idempotent and
> thread-safe, to be called from multiple threads.  Consider:
> 
> app links against liba and libb;
> liba links against libqb
> libb links against libqb
> 
> How am I supposed to write liba and libb to guarantee only one single
> race-free call to libqblock_init, unless libqblock_init() is idempotent?

I agree, libqblock_init should use pthread_once (or we can add QemuOnce
to qemu-thread-*.c).

Paolo

> Il 04/09/2012 05:15, Wenchao Xia ha scritto:
>>>
>>> Can you use GError instead?
>>>
>>    read through the GError doc, GError is defined as following:
>> struct GError {
>>    GQuark       domain;
>>    gint         code;
>>    gchar       *message;
>> };
>>    I am worried about the message member, I guess program would be
>> aborted if OOM, which I was tring to avoid, so I used char err_msg[1024]
>> in my code, and make things simpler.
>
> That's true.  On the other hand, and IMHO, not aborting in the library
> code is a non-goal as long as the rest of the block layer still does.
>
   About the Gerror lib, with a look at its doc, I think it provides
similar capabilities with my implement, no key feature provided.
Considering the memory issue, I hope to drop Gerror now.

>>>>     3 QBlockInfoImageStatic. Now it is not folded with location and
>>>> format.
>>>
>>> What does "Static" mean?
>>>
>>   It is about sorting the information into following kinds:
>> 1) static. It is values that defined at creating time/modifying time,
>> mostly some settings, and it would not be automatically changed in I/O.
>> 2) dynamic. Some information that would changes in I/O and other
>> operations, such as allocated_size, snapshots.
>> 3) statistics.
>>    Now only static one is provided, so I added _static suffix.
>
> Makes sense, thanks for the clarification.  Perhaps QBlockStaticInfo is
> a shorter and simpler name?
>
> Paolo
>