Re: [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats
On Fri, Jul 8, 2011 at 11:29 PM, Stefan Hajnoczi wrote:
> On Tue, Jul 5, 2011 at 12:31 PM, Fam Zheng wrote:
>> Add create option 'format', with enums:
>
> The -drive format=... option exists in QEMU today to specify the image
> format of a file. I think adding a format=... creation option may
> lead to confusion.
>
> How about subformat=... or type=...?
>
>> Each creates a subformat image file. The default is monolithiSparse.
>
> s/monolithiSparse/monolithicSparse/
>
>> @@ -243,168 +243,6 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
>> return 1;
>> }
>>
>> -static int vmdk_snapshot_create(const char *filename, const char
>> *backing_file)
>
> Is this function really not needed anymore?
>
I think so. This function is not used to create snapshot, actually
it's called for backing file. It copies header and L1 table, allocate
L2 tables and leave them zero. This is equivalent to creating a new
extent. The only difference is to set descriptor options parentCID and
parantFileNameHint.
>> @@ -1189,28 +990,317 @@ static int vmdk_create(const char *filename,
>> QEMUOptionParameter *options)
>> }
>> }
>>
>> - /* compose the descriptor */
>> - real_filename = filename;
>> - if ((temp_str = strrchr(real_filename, '\\')) != NULL)
>> - real_filename = temp_str + 1;
>> - if ((temp_str = strrchr(real_filename, '/')) != NULL)
>> - real_filename = temp_str + 1;
>> - if ((temp_str = strrchr(real_filename, ':')) != NULL)
>> - real_filename = temp_str + 1;
>> - snprintf(desc, sizeof(desc), desc_template, (unsigned int)time(NULL),
>> - total_size, real_filename,
>> - (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
>> - total_size / (int64_t)(63 * 16));
>> -
>> - /* write the descriptor */
>> - lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
>> - ret = qemu_write_full(fd, desc, strlen(desc));
>> - if (ret != strlen(desc)) {
>> + filesize -= filesize;
>
> What is the point of setting filesize to zero?
>
>> + ret = 0;
>> + exit:
>> + close(fd);
>> + return ret;
>> +}
>> +
>> +static int vmdk_create_flat(const char *filename, int64_t filesize)
>> +{
>> + int fd, ret;
>> +
>> + fd = open(
>> + filename,
>> + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> + 0644);
>> + if (fd < 0) {
>> + return -errno;
>> + }
>> + ret = ftruncate(fd, filesize);
>> + if (ret) {
>> ret = -errno;
>> - goto exit;
>> + close(fd);
>> + return -errno;
>
> errno is a global variable that may be modified by any errno-using
> library function. Its value may be changed by close(2) (even if there
> is no error closing the fd). Therefore please do return ret instead
> of return -errno.
>
>> }
>> + close(fd);
>> + return 0;
>> +}
>>
>> - ret = 0;
>> +static int filename_decompose(const char *filename, char *path, char
>> *prefix,
>> + char *postfix, int buf_len)
>
> Memory sizes (e.g. buffer size) should be size_t (which is unsigned)
> instead of int.
>
>> +{
>> + const char *p, *q;
>> +
>> + if (filename == NULL || !strlen(filename)) {
>> + fprintf(stderr, "Vmdk: wrong filename (%s)\n", filename);
>
> Printing filename doesn't make sense since filename is either NULL or
> "". Also note that fprintf(..., "%s", NULL) is undefined and may
> crash on some platforms (e.g. Solaris).
>
>> + return -1;
>> + }
>> + p = strrchr(filename, '/');
>> + if (p == NULL) {
>> + p = strrchr(filename, '\\');
>> + }
>> + if (p == NULL) {
>> + p = strrchr(filename, ':');
>> + }
>> + if (p != NULL) {
>> + p++;
>> + if (p - filename >= buf_len) {
>> + return -1;
>> + }
>> + strncpy(path, filename, p - filename);
>> + path[p - filename] = 0;
>> + } else {
>> + p = filename;
>> + path[0] = '\0';
>> + }
>> + q = strrchr(p, '.');
>> + if (q == NULL) {
>> + pstrcpy(prefix, buf_len, p);
>> + postfix[0] = '\0';
>> + } else {
>
> No check for prefix buf_len here. Imagine filename has no '/', '\\',
> or ':' but it does have a '.'. It is possible to overflow prefix.
>
>> + strncpy(prefix, p, q - p);
>> + prefix[q - p] = '\0';
>> + pstrcpy(postfix, buf_len, q);
>> + }
>> + return 0;
>> +}
>> +
>> +static int relative_path(char *dest, int dest_size,
>> + const char *base, const char *target)
>> +{
>> + int i = 0;
>> + int n = 0;
>> + const char *p, *q;
>> +#ifdef _WIN32
>> + const char *sep = "\\";
>> +#else
>> + const char *sep = "/";
>> +#endif
>> +
>> + if (!(dest && base && target)) {
>> + return -1;
>> + }
>> + if (path_is_absolute(target)) {
>> + dest[dest_size - 1] = '\0';
>> + strncpy(dest, target, dest_size - 1);
>> + return 0;
>> + }
>> + while (base[i] == target[i]) {
>> + i++;
>> + }
>> + p =
Re: [Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats
On Tue, Jul 5, 2011 at 12:31 PM, Fam Zheng wrote:
> Add create option 'format', with enums:
The -drive format=... option exists in QEMU today to specify the image
format of a file. I think adding a format=... creation option may
lead to confusion.
How about subformat=... or type=...?
> Each creates a subformat image file. The default is monolithiSparse.
s/monolithiSparse/monolithicSparse/
> @@ -243,168 +243,6 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
> return 1;
> }
>
> -static int vmdk_snapshot_create(const char *filename, const char
> *backing_file)
Is this function really not needed anymore?
> @@ -1189,28 +990,317 @@ static int vmdk_create(const char *filename,
> QEMUOptionParameter *options)
> }
> }
>
> - /* compose the descriptor */
> - real_filename = filename;
> - if ((temp_str = strrchr(real_filename, '\\')) != NULL)
> - real_filename = temp_str + 1;
> - if ((temp_str = strrchr(real_filename, '/')) != NULL)
> - real_filename = temp_str + 1;
> - if ((temp_str = strrchr(real_filename, ':')) != NULL)
> - real_filename = temp_str + 1;
> - snprintf(desc, sizeof(desc), desc_template, (unsigned int)time(NULL),
> - total_size, real_filename,
> - (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
> - total_size / (int64_t)(63 * 16));
> -
> - /* write the descriptor */
> - lseek(fd, le64_to_cpu(header.desc_offset) << 9, SEEK_SET);
> - ret = qemu_write_full(fd, desc, strlen(desc));
> - if (ret != strlen(desc)) {
> + filesize -= filesize;
What is the point of setting filesize to zero?
> + ret = 0;
> + exit:
> + close(fd);
> + return ret;
> +}
> +
> +static int vmdk_create_flat(const char *filename, int64_t filesize)
> +{
> + int fd, ret;
> +
> + fd = open(
> + filename,
> + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> + 0644);
> + if (fd < 0) {
> + return -errno;
> + }
> + ret = ftruncate(fd, filesize);
> + if (ret) {
> ret = -errno;
> - goto exit;
> + close(fd);
> + return -errno;
errno is a global variable that may be modified by any errno-using
library function. Its value may be changed by close(2) (even if there
is no error closing the fd). Therefore please do return ret instead
of return -errno.
> }
> + close(fd);
> + return 0;
> +}
>
> - ret = 0;
> +static int filename_decompose(const char *filename, char *path, char *prefix,
> + char *postfix, int buf_len)
Memory sizes (e.g. buffer size) should be size_t (which is unsigned)
instead of int.
> +{
> + const char *p, *q;
> +
> + if (filename == NULL || !strlen(filename)) {
> + fprintf(stderr, "Vmdk: wrong filename (%s)\n", filename);
Printing filename doesn't make sense since filename is either NULL or
"". Also note that fprintf(..., "%s", NULL) is undefined and may
crash on some platforms (e.g. Solaris).
> + return -1;
> + }
> + p = strrchr(filename, '/');
> + if (p == NULL) {
> + p = strrchr(filename, '\\');
> + }
> + if (p == NULL) {
> + p = strrchr(filename, ':');
> + }
> + if (p != NULL) {
> + p++;
> + if (p - filename >= buf_len) {
> + return -1;
> + }
> + strncpy(path, filename, p - filename);
> + path[p - filename] = 0;
> + } else {
> + p = filename;
> + path[0] = '\0';
> + }
> + q = strrchr(p, '.');
> + if (q == NULL) {
> + pstrcpy(prefix, buf_len, p);
> + postfix[0] = '\0';
> + } else {
No check for prefix buf_len here. Imagine filename has no '/', '\\',
or ':' but it does have a '.'. It is possible to overflow prefix.
> + strncpy(prefix, p, q - p);
> + prefix[q - p] = '\0';
> + pstrcpy(postfix, buf_len, q);
> + }
> + return 0;
> +}
> +
> +static int relative_path(char *dest, int dest_size,
> + const char *base, const char *target)
> +{
> + int i = 0;
> + int n = 0;
> + const char *p, *q;
> +#ifdef _WIN32
> + const char *sep = "\\";
> +#else
> + const char *sep = "/";
> +#endif
> +
> + if (!(dest && base && target)) {
> + return -1;
> + }
> + if (path_is_absolute(target)) {
> + dest[dest_size - 1] = '\0';
> + strncpy(dest, target, dest_size - 1);
> + return 0;
> + }
> + while (base[i] == target[i]) {
> + i++;
> + }
> + p = &base[i];
> + q = &target[i];
> + while (*p) {
> + if (*p == *sep) {
> + n++;
> + }
> + p++;
> + }
> + dest[0] = '\0';
> + for (; n; n--) {
> + pstrcat(dest, dest_size, "..");
> + pstrcat(dest, dest_size, sep);
> + }
> + pstrcat(dest, dest_size, q);
> + return 0;
> +}
> +
> +static int vmdk_create(const char *filename, QEMUOptionParameter *options)
> +{
> + int fd = -1;
> + char desc[4096];
> + int64_t total_size = 0;
> + const char *backing_file =
[Qemu-devel] [PATCH v8 10/12] VMDK: create different subformats
Add create option 'format', with enums:
monolithicSparse
monolithicFlat
twoGbMaxExtentSparse
twoGbMaxExtentFlat
Each creates a subformat image file. The default is monolithiSparse.
Signed-off-by: Fam Zheng
---
block/vmdk.c | 561 ++
block_int.h |1 +
2 files changed, 330 insertions(+), 232 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 2183ace..0f51332 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -156,8 +156,8 @@ static int vmdk_probe(const uint8_t *buf, int buf_size,
const char *filename)
#define CHECK_CID 1
#define SECTOR_SIZE 512
-#define DESC_SIZE 20*SECTOR_SIZE // 20 sectors of 512 bytes each
-#define HEADER_SIZE 512// first sector of 512 bytes
+#define DESC_SIZE (20 * SECTOR_SIZE)/* 20 sectors of 512 bytes each */
+#define HEADER_SIZE 512 /* first sector of 512 bytes */
static void vmdk_free_extents(BlockDriverState *bs)
{
@@ -243,168 +243,6 @@ static int vmdk_is_cid_valid(BlockDriverState *bs)
return 1;
}
-static int vmdk_snapshot_create(const char *filename, const char *backing_file)
-{
-int snp_fd, p_fd;
-int ret;
-uint32_t p_cid;
-char *p_name, *gd_buf, *rgd_buf;
-const char *real_filename, *temp_str;
-VMDK4Header header;
-uint32_t gde_entries, gd_size;
-int64_t gd_offset, rgd_offset, capacity, gt_size;
-char p_desc[DESC_SIZE], s_desc[DESC_SIZE], hdr[HEADER_SIZE];
-static const char desc_template[] =
-"# Disk DescriptorFile\n"
-"version=1\n"
-"CID=%x\n"
-"parentCID=%x\n"
-"createType=\"monolithicSparse\"\n"
-"parentFileNameHint=\"%s\"\n"
-"\n"
-"# Extent description\n"
-"RW %u SPARSE \"%s\"\n"
-"\n"
-"# The Disk Data Base \n"
-"#DDB\n"
-"\n";
-
-snp_fd = open(filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY |
O_LARGEFILE, 0644);
-if (snp_fd < 0)
-return -errno;
-p_fd = open(backing_file, O_RDONLY | O_BINARY | O_LARGEFILE);
-if (p_fd < 0) {
-close(snp_fd);
-return -errno;
-}
-
-/* read the header */
-if (lseek(p_fd, 0x0, SEEK_SET) == -1) {
-ret = -errno;
-goto fail;
-}
-if (read(p_fd, hdr, HEADER_SIZE) != HEADER_SIZE) {
-ret = -errno;
-goto fail;
-}
-
-/* write the header */
-if (lseek(snp_fd, 0x0, SEEK_SET) == -1) {
-ret = -errno;
-goto fail;
-}
-if (write(snp_fd, hdr, HEADER_SIZE) == -1) {
-ret = -errno;
-goto fail;
-}
-
-memset(&header, 0, sizeof(header));
-memcpy(&header,&hdr[4], sizeof(header)); // skip the VMDK4_MAGIC
-
-if (ftruncate(snp_fd, header.grain_offset << 9)) {
-ret = -errno;
-goto fail;
-}
-/* the descriptor offset = 0x200 */
-if (lseek(p_fd, 0x200, SEEK_SET) == -1) {
-ret = -errno;
-goto fail;
-}
-if (read(p_fd, p_desc, DESC_SIZE) != DESC_SIZE) {
-ret = -errno;
-goto fail;
-}
-
-if ((p_name = strstr(p_desc,"CID")) != NULL) {
-p_name += sizeof("CID");
-sscanf(p_name,"%x",&p_cid);
-}
-
-real_filename = filename;
-if ((temp_str = strrchr(real_filename, '\\')) != NULL)
-real_filename = temp_str + 1;
-if ((temp_str = strrchr(real_filename, '/')) != NULL)
-real_filename = temp_str + 1;
-if ((temp_str = strrchr(real_filename, ':')) != NULL)
-real_filename = temp_str + 1;
-
-snprintf(s_desc, sizeof(s_desc), desc_template, p_cid, p_cid, backing_file,
- (uint32_t)header.capacity, real_filename);
-
-/* write the descriptor */
-if (lseek(snp_fd, 0x200, SEEK_SET) == -1) {
-ret = -errno;
-goto fail;
-}
-if (write(snp_fd, s_desc, strlen(s_desc)) == -1) {
-ret = -errno;
-goto fail;
-}
-
-gd_offset = header.gd_offset * SECTOR_SIZE; // offset of GD table
-rgd_offset = header.rgd_offset * SECTOR_SIZE; // offset of RGD table
-capacity = header.capacity * SECTOR_SIZE; // Extent size
-/*
- * Each GDE span 32M disk, means:
- * 512 GTE per GT, each GTE points to grain
- */
-gt_size = (int64_t)header.num_gtes_per_gte * header.granularity *
SECTOR_SIZE;
-if (!gt_size) {
-ret = -EINVAL;
-goto fail;
-}
-gde_entries = (uint32_t)(capacity / gt_size); // number of gde/rgde
-gd_size = gde_entries * sizeof(uint32_t);
-
-/* write RGD */
-rgd_buf = qemu_malloc(gd_size);
-if (lseek(p_fd, rgd_offset, SEEK_SET) == -1) {
-ret = -errno;
-goto fail_rgd;
-}
-if (read(p_fd, rgd_buf, gd_size) != gd_size) {
-ret = -errno;
-goto fail_rgd;
-}
-if (lseek(snp_fd, rgd_offset, SEEK_SET) == -1) {
-ret = -errno;
-goto fail_rgd;
-}
-if (write(snp_fd, rgd_buf, gd_size) == -1) {
-ret = -errno;
-goto fail_rgd;
-}
-
-/* write GD */
-gd_buf =
