Re: [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol

[Fam Zheng]

On 2013年12月19日 21:12, Stefan Hajnoczi wrote:
> On Tue, Dec 17, 2013 at 08:00:00PM +0800, Fam Zheng wrote:
> > @@ -1511,48 +1521,55 @@ static int vmdk_create_extent(const char *filename, 
> > int64_t filesize,
> >       header.check_bytes[3] = 0xa;
> >
> >       /* write all the data */
> > -    ret = qemu_write_full(fd, &magic, sizeof(magic));
> > -    if (ret != sizeof(magic)) {
> > -        ret = -errno;
> > +    ret = bdrv_pwrite(bs, 0, &magic, sizeof(magic));
> > +    if (ret < 0) {
> > +        error_set(errp, QERR_IO_ERROR);
> >           goto exit;
> >       }
> > -    ret = qemu_write_full(fd, &header, sizeof(header));
> > +    ret = bdrv_pwrite(bs, sizeof(magic), &header, sizeof(header));
> >       if (ret != sizeof(header)) {
> > +        error_set(errp, QERR_IO_ERROR);
> >           ret = -errno;
>
> This line should be deleted.
>
> Also, I noticed you changed ret != sizeof(magic) to ret < 0 for the
> magic number bdrv_pwrite() but did not change the condition for the
> header write.  Please keep the error handling condition consistent.

OK.

> >           goto exit;
> >       }
> >
> > -    ret = ftruncate(fd, le64_to_cpu(header.grain_offset) << 9);
> > +    ret = bdrv_truncate(bs, (le64_to_cpu(header.grain_offset)) << 9);
>
> Why add parentheses around le64_to_cpu()?

Unintended. Will remove.

> >       if (ret < 0) {
> > -        ret = -errno;
> > -        goto exit;
> > +        error_setg(errp, "Could not truncate file");
>
> goto exit?

Yes, thanks.

> >       }
> >
> >       /* write grain directory */
> > -    lseek(fd, le64_to_cpu(header.rgd_offset) << 9, SEEK_SET);
> > -    for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_size;
> > +    gd_buf_size = gd_sectors * BDRV_SECTOR_SIZE * sizeof(*gd_buf);
> > +    gd_buf = g_malloc0(gd_buf_size);
> > +    for (i = 0, tmp = le64_to_cpu(header.rgd_offset) + gd_sectors;
> >            i < gt_count; i++, tmp += gt_size) {
> > -        ret = qemu_write_full(fd, &tmp, sizeof(tmp));
> > -        if (ret != sizeof(tmp)) {
> > -            ret = -errno;
> > -            goto exit;
> > -        }
>
> Was this old code not endian-safe?  It appears to be writing native
> endian values.  The new code is different.

Yes, a bonus bug fix. I'll add note in the commit message.

> > @@ -1771,33 +1791,34 @@ static int vmdk_create(const char *filename, 
> > QEMUOptionParameter *options,
> >                              total_size / (int64_t)(63 * number_heads * 512),
> >                              number_heads,
> >                              adapter_type);
> > -    if (split || flat) {
> > -        fd = qemu_open(filename,
> > -                       O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> > -                       0644);
> > +    desc_len = strlen(desc);
> > +    /* the descriptor offset = 0x200 */
> > +    if (!split && !flat) {
> > +        desc_offset = 0x200;
> >       } else {
> > -        fd = qemu_open(filename,
> > -                       O_WRONLY | O_BINARY | O_LARGEFILE,
> > -                       0644);
> > +        ret = bdrv_create_file(filename, options, &local_err);
>
> Missing error handling if bdrv_create_file() fails.

OK.

> >       }
> > -    if (fd < 0) {
> > -        ret = -errno;
> > +    ret = bdrv_file_open(&new_bs, filename, NULL, BDRV_O_RDWR, &local_err);
> > +    if (ret < 0) {
> > +        error_setg_errno(errp, -ret, "Could not write description");
> >           goto exit;
> >       }
> > -    /* the descriptor offset = 0x200 */
> > -    if (!split && !flat && 0x200 != lseek(fd, 0x200, SEEK_SET)) {
> > -        ret = -errno;
> > -        goto close_exit;
> > +    ret = bdrv_pwrite(new_bs, desc_offset, desc, desc_len);
> > +    if (ret < 0) {
> > +        error_setg_errno(errp, -ret, "Could not write description");
>
> goto close_exit?

OK.

> >       }
> > -    ret = qemu_write_full(fd, desc, strlen(desc));
> > -    if (ret != strlen(desc)) {
> > -        ret = -errno;
> > -        goto close_exit;
> > +    /* bdrv_pwrite write padding zeros to align to sector, we don't need that
> > +     * for description file */
> > +    if (desc_offset == 0) {
> > +        ret = bdrv_truncate(new_bs, desc_offset + desc_len);
>
> We know desc_offset == 0, so desc_offset (0) + desc_len is really just
> desc_len.

OK.

Thanks for the review!

Fam