δΊ 2012-11-20 18:41, Paolo Bonzini ει:
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.
It is first created in 4/7 build system patch, should it be changed or
add API defines into that patch?
I dislike QBlockState and would prefer something like QBlockImage...
OK, QBlockImage seems good to me too.
Also, it would be nicer to use reference counting for QBlockImage,
rather than just a new/delete pair.
OK, will add.
There are some spelling mistakes (openned->opened).
My mistake, will fix it.
But overall we're converging, it's good work that you've done. Thanks
for persisting!
It is my pleasure, thank u for carefully reviewing the patch.
+ 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.
OK.
+ 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;
+ }
...
This check is done to avoid silent fail in translating bytes to
sector number used in API
bdrv_is_allocated(bs, sector_start, sector_num, &num)
sector_num is uint_32, so this length should be no larger than
uint32<<BDRV_SECTOR_MASK.
Sorry this section had some problem and lacks document, make it
looks joke.... Will fix the check with precise size.
+ 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.
OK.
+ bs = qbs->bdrvs;
+
+ ret = filename2loc(context,
+ &(info_tmp->loc),
Useless parentheses around & argument. This is very common, please
remove them.
OK.
+ 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.
OK.
+ member_addr = &addr;
+ qb_setup_info_addr(info_tmp, member_addr);
Just use qb_setup_info_addr(info_tmp, &addr).
OK.
+
+ 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.
OK.
+ 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;
+}
--
Best Regards
Wenchao Xia