diff mbox

vmdk: Allow vmdk_create to work with protocol

Message ID 1387281600-2111-1-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Dec. 17, 2013, noon UTC 
This changes vmdk_create to use bdrv_* functions to replace qemu_open
and other fd functions. The error handling are improved as well. One
difference is that bdrv_pwrite will round up buffer to sectors, so for
description file, an extra bdrv_truncate is used in the end to drop
ending zeros.

I tested that new code produces exactly the same file as previously.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 137 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 79 insertions(+), 58 deletions(-)

Comments

Stefan Hajnoczi Dec. 19, 2013, 1:12 p.m. UTC | #1 
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.
Fam Zheng Dec. 20, 2013, 1:48 a.m. UTC | #2 
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
diff mbox

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index 0734bc2..76efa38 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1447,23 +1447,33 @@  static int coroutine_fn vmdk_co_write_zeroes(BlockDriverState *bs,
 }
 
 static int vmdk_create_extent(const char *filename, int64_t filesize,
-                              bool flat, bool compress, bool zeroed_grain)
+                              bool flat, bool compress, bool zeroed_grain,
+                              Error **errp)
 {
     int ret, i;
-    int fd = 0;
+    BlockDriverState *bs = NULL;
     VMDK4Header header;
-    uint32_t tmp, magic, grains, gd_size, gt_size, gt_count;
+    Error *local_err;
+    uint32_t tmp, magic, grains, gd_sectors, gt_size, gt_count;
+    uint32_t *gd_buf = NULL;
+    int gd_buf_size;
 
-    fd = qemu_open(filename,
-                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
-                   0644);
-    if (fd < 0) {
-        return -errno;
+    ret = bdrv_create_file(filename, NULL, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        goto exit;
     }
+
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        goto exit;
+    }
+
     if (flat) {
-        ret = ftruncate(fd, filesize);
+        ret = bdrv_truncate(bs, filesize);
         if (ret < 0) {
-            ret = -errno;
+            error_setg(errp, "Could not truncate file");
         }
         goto exit;
     }
@@ -1482,14 +1492,14 @@  static int vmdk_create_extent(const char *filename, int64_t filesize,
     gt_size = ((header.num_gtes_per_gt * sizeof(uint32_t)) + 511) >> 9;
     gt_count =
         (grains + header.num_gtes_per_gt - 1) / header.num_gtes_per_gt;
-    gd_size = (gt_count * sizeof(uint32_t) + 511) >> 9;
+    gd_sectors = (gt_count * sizeof(uint32_t) + 511) >> 9;
 
     header.desc_offset = 1;
     header.desc_size = 20;
     header.rgd_offset = header.desc_offset + header.desc_size;
-    header.gd_offset = header.rgd_offset + gd_size + (gt_size * gt_count);
+    header.gd_offset = header.rgd_offset + gd_sectors + (gt_size * gt_count);
     header.grain_offset =
-       ((header.gd_offset + gd_size + (gt_size * gt_count) +
+       ((header.gd_offset + gd_sectors + (gt_size * gt_count) +
          header.granularity - 1) / header.granularity) *
         header.granularity;
     /* swap endianness for all header fields */
@@ -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;
         goto exit;
     }
 
-    ret = ftruncate(fd, le64_to_cpu(header.grain_offset) << 9);
+    ret = bdrv_truncate(bs, (le64_to_cpu(header.grain_offset)) << 9);
     if (ret < 0) {
-        ret = -errno;
-        goto exit;
+        error_setg(errp, "Could not truncate file");
     }
 
     /* 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;
-        }
+        gd_buf[i] = cpu_to_le32(tmp);
+    }
+    ret = bdrv_pwrite(bs, le64_to_cpu(header.rgd_offset) * BDRV_SECTOR_SIZE,
+                      gd_buf, gd_buf_size);
+    if (ret < 0) {
+        error_set(errp, QERR_IO_ERROR);
+        goto exit;
     }
 
     /* write backup grain directory */
-    lseek(fd, le64_to_cpu(header.gd_offset) << 9, SEEK_SET);
-    for (i = 0, tmp = le64_to_cpu(header.gd_offset) + gd_size;
+    for (i = 0, tmp = le64_to_cpu(header.gd_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;
-        }
+        gd_buf[i] = cpu_to_le32(tmp);
+    }
+    ret = bdrv_pwrite(bs, le64_to_cpu(header.gd_offset) * BDRV_SECTOR_SIZE,
+                      gd_buf, gd_buf_size);
+    if (ret < 0) {
+        error_set(errp, QERR_IO_ERROR);
+        goto exit;
     }
 
     ret = 0;
- exit:
-    qemu_close(fd);
+exit:
+    if (bs) {
+        bdrv_unref(bs);
+    }
+    g_free(gd_buf);
     return ret;
 }
 
@@ -1599,7 +1616,9 @@  static int filename_decompose(const char *filename, char *path, char *prefix,
 static int vmdk_create(const char *filename, QEMUOptionParameter *options,
                        Error **errp)
 {
-    int fd, idx = 0;
+    int idx = 0;
+    BlockDriverState *new_bs = NULL;
+    Error *local_err;
     char *desc = NULL;
     int64_t total_size = 0, filesize;
     const char *adapter_type = NULL;
@@ -1616,6 +1635,7 @@  static int vmdk_create(const char *filename, QEMUOptionParameter *options,
     uint32_t parent_cid = 0xffffffff;
     uint32_t number_heads = 16;
     bool zeroed_grain = false;
+    uint32_t desc_offset = 0, desc_len;
     const char desc_template[] =
         "# Disk DescriptorFile\n"
         "version=1\n"
@@ -1749,7 +1769,7 @@  static int vmdk_create(const char *filename, QEMUOptionParameter *options,
                 path, desc_filename);
 
         if (vmdk_create_extent(ext_filename, size,
-                               flat, compress, zeroed_grain)) {
+                               flat, compress, zeroed_grain, errp)) {
             ret = -EINVAL;
             goto exit;
         }
@@ -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);
     }
-    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");
     }
-    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);
+        if (ret < 0) {
+            error_setg(errp, "Could not truncate file");
+        }
     }
-    ret = 0;
-close_exit:
-    qemu_close(fd);
 exit:
+    if (new_bs) {
+        bdrv_unref(new_bs);
+    }
     g_free(desc);
     g_string_free(ext_desc_lines, true);
     return ret;