* [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol
@ 2013-12-17 12:00 Fam Zheng
2013-12-19 13:12 ` Stefan Hajnoczi
0 siblings, 1 reply; 3+ messages in thread
From: Fam Zheng @ 2013-12-17 12:00 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, pl, stefanha
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(-)
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;
--
1.8.5.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol
2013-12-17 12:00 [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol Fam Zheng
@ 2013-12-19 13:12 ` Stefan Hajnoczi
2013-12-20 1:48 ` Fam Zheng
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Hajnoczi @ 2013-12-19 13:12 UTC (permalink / raw)
To: Fam Zheng; +Cc: kwolf, pl, qemu-devel, stefanha
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol
2013-12-19 13:12 ` Stefan Hajnoczi
@ 2013-12-20 1:48 ` Fam Zheng
0 siblings, 0 replies; 3+ messages in thread
From: Fam Zheng @ 2013-12-20 1:48 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pl, qemu-devel, stefanha
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-12-20 1:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-17 12:00 [Qemu-devel] [PATCH] vmdk: Allow vmdk_create to work with protocol Fam Zheng
2013-12-19 13:12 ` Stefan Hajnoczi
2013-12-20 1:48 ` Fam Zheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).