From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40967) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TalHI-0000Wr-8n for qemu-devel@nongnu.org; Tue, 20 Nov 2012 05:42:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TalHG-0006A5-MV for qemu-devel@nongnu.org; Tue, 20 Nov 2012 05:42:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TalHG-0006A0-E4 for qemu-devel@nongnu.org; Tue, 20 Nov 2012 05:42:02 -0500 Message-ID: <50AB5E61.9060303@redhat.com> Date: Tue, 20 Nov 2012 11:41:37 +0100 From: Paolo Bonzini 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> In-Reply-To: <1353404767-4495-7-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V10 6/7] libqblock API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia 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 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; > +}