From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38745) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb1CG-0001gi-Ue for qemu-devel@nongnu.org; Tue, 20 Nov 2012 22:41:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tb1CF-0007Wh-IV for qemu-devel@nongnu.org; Tue, 20 Nov 2012 22:41:56 -0500 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:56399) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tb1CE-0007VH-LV for qemu-devel@nongnu.org; Tue, 20 Nov 2012 22:41:55 -0500 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 21 Nov 2012 09:11:51 +0530 Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qAL3fjFb6619594 for ; Wed, 21 Nov 2012 09:11:46 +0530 Received: from d28av01.in.ibm.com (loopback [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qAL9Ban9031742 for ; Wed, 21 Nov 2012 09:11:37 GMT Message-ID: <50AC4D37.1000809@linux.vnet.ibm.com> Date: Wed, 21 Nov 2012 11:40:39 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1353404767-4495-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1353404767-4495-7-git-send-email-xiawenc@linux.vnet.ibm.com> <50AB5E61.9060303@redhat.com> In-Reply-To: <50AB5E61.9060303@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V10 6/7] libqblock API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, peter.maydell@linaro.org, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, kraxel@redhat.com 于 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< >> + 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