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