[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V10 6/7] libqblock API
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH V10 6/7] libqblock API |
Date: |
Tue, 20 Nov 2012 11:41:37 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121016 Thunderbird/16.0.1 |
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;
> +}
- Re: [Qemu-devel] [PATCH V10 4/7] libqblock build system, (continued)
[Qemu-devel] [PATCH V10 5/7] libqblock type defines, Wenchao Xia, 2012/11/20
[Qemu-devel] [PATCH V10 6/7] libqblock API, Wenchao Xia, 2012/11/20
- Re: [Qemu-devel] [PATCH V10 6/7] libqblock API,
Paolo Bonzini <=
[Qemu-devel] [PATCH V10 7/7] libqblock test example, Wenchao Xia, 2012/11/20