Re: [Qemu-devel] [PATCH V10 6/7] libqblock API

[Wenchao Xia]

于 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