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

[Stefan Hajnoczi]

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.

>          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()?

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

goto exit?

>      }
>  
>      /* 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.

> @@ -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.

>      }
> -    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?

>      }
> -    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.