Re: [Qemu-devel] [PATCH V10 6/7] libqblock API

[Paolo Bonzini <pbonzini@redhat.com>]

Il 20/11/2012 10:46, Wenchao Xia ha scritto:
> diff --git a/libqblock/libqblock.h b/libqblock/libqblock.h
> index e69de29..8ca7d28 100644
> --- a/libqblock/libqblock.h
> +++ b/libqblock/libqblock.h

Please move the creation of this file to the previous patch.

I dislike QBlockState and would prefer something like QBlockImage...

Also, it would be nicer to use reference counting for QBlockImage,
rather than just a new/delete pair.

There are some spelling mistakes (openned->opened).

But overall we're converging, it's good work that you've done.  Thanks
for persisting!

> 
> +    byte_offset = offset & (~BDRV_SECTOR_MASK);
> +    if (byte_offset != 0) {
> +        /* the start sector is not alligned, need to read/write this sector. */
> +        bd_ret = bdrv_read(bs, sector_start, temp_buf, 1);
> +        if (bd_ret < 0) {
> +            set_context_err(context, QB_ERR_INTERNAL_ERR,
> +                           "QEMU internal block error.");

These are I/O errors, not internal errors.

> 
> +    if (length > 0x1000000000000) {

What is this magic number?  I think, just remove the check.

> +        set_context_err(context, QB_ERR_INVALID_PARAM,
> +                       "length is too big.");
> +        goto out;
> +    }

...

> +            context->err_no = bd_ret;
> +            return context->err_ret;
> +        }
> +        cp_len = BDRV_SECTOR_SIZE - byte_offset;
> +        memcpy(p, temp_buf + byte_offset, cp_len);
> +
> +        remains -= cp_len;
> +        p += cp_len;
> +        sector_start++;
> +    }
> +
> +    /* now start position is alligned. */
> +    if (remains >= BDRV_SECTOR_SIZE) {
> +        sector_num = remains >> BDRV_SECTOR_BITS;
> +        bd_ret = bdrv_read(bs, sector_start, p, sector_num);
> +        if (bd_ret < 0) {
> +            set_context_err(context, QB_ERR_INTERNAL_ERR,
> +                           "QEMU internal block error.");
> +            context->err_no = bd_ret;
> +            return context->err_ret;
> +        }
> +        remains -= sector_num << BDRV_SECTOR_BITS;
> +        p += sector_num << BDRV_SECTOR_BITS;
> +        sector_start += sector_num;
> +    }
> +
> +    if (remains > 0) {
> +        /* there is some request remains, less than 1 sector */
> +        bd_ret = bdrv_read(bs, sector_start, temp_buf, 1);
> +        if (bd_ret < 0) {
> +            set_context_err(context, QB_ERR_INTERNAL_ERR,
> +                           "QEMU internal block error.");
> +            context->err_no = bd_ret;
> +            return context->err_ret;
> +        }
> +        memcpy(p, temp_buf, remains);
> +        remains -= remains;
> +    }
> +



> +int qb_info_image_static_get(QBlockContext *context,
> +                             QBlockState *qbs,
> +                             QBlockStaticInfo **info)
> +{
> +    int ret = 0;
> +    BlockDriverState *bs;
> +    QBlockStaticInfo *info_tmp;
> +    QBlockStaticInfoAddr *member_addr, addr;
> +    const char *fmt_str;
> +    uint64_t total_sectors;
> +    char backing_filename[LIBQB_FILENAME_MAX];
> +
> +    if (qbs->filename == NULL) {
> +        set_context_err(context, QB_ERR_INVALID_PARAM,
> +                       "Block Image was not openned.");
> +        ret = context->err_ret;
> +        goto out;
> +    }
> +
> +    info_tmp = FUNC_CALLOC(1, sizeof(QBlockStaticInfo));

Please rename info to p_info and info_tmp to info.

> +    bs = qbs->bdrvs;
> +
> +    ret = filename2loc(context,
> +                       &(info_tmp->loc),

Useless parentheses around & argument.  This is very common, please
remove them.

> +                       qbs->filename);
> +    if (ret < 0) {
> +        goto free;
> +    }
> +
> +    fmt_str = bdrv_get_format_name(bs);
> +    info_tmp->fmt.fmt_type = qb_str2fmttype(fmt_str);
> +    /* we got the format type and basic location info now, setup the struct
> +    pointer to the internal members */
> +    memset(&addr, 0, sizeof(QBlockStaticInfoAddr));

Please move these memsets to qb_setup_info_addr.

> +    member_addr = &addr;
> +    qb_setup_info_addr(info_tmp, member_addr);

Just use qb_setup_info_addr(info_tmp, &addr).

> +
> +    assert(member_addr->virt_size != NULL);
> +    bdrv_get_geometry(bs, &total_sectors);
> +    *(member_addr->virt_size) = total_sectors * BDRV_SECTOR_SIZE;

virt_size is always valid, please move it out of the unions and in the
main struct.

> +    if (member_addr->encrypt != NULL) {
> +        *(member_addr->encrypt) = bdrv_is_encrypted(bs);
> +    }
> +    bdrv_get_full_backing_filename(bs, backing_filename,
> +                                   sizeof(backing_filename));
> +    if (backing_filename[0] != '\0') {
> +        assert(member_addr->backing_loc != NULL);
> +        ret = filename2loc(context,
> +                           member_addr->backing_loc,
> +                           backing_filename);
> +        if (ret < 0) {
> +            goto free;
> +        }
> +    }
> +
> +    info_tmp->sector_size = BDRV_SECTOR_SIZE;
> +    *info = info_tmp;
> +
> + out:
> +    return ret;
> + free:
> +    qb_info_image_static_delete(context, &info_tmp);
> +    return ret;
> +}