* [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes
@ 2014-12-03 10:28 Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID Fam Zheng
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
v3: Fix 3/6 again. (Markus)
v2: 3/6: Don't overwrite the last byte of buffer. (Don)
Other patches are unchanged (added Markus' rev-by line).
Here are some improvements on miscellaneous things such as CID generation,
comments, input validation.
Fam Zheng (6):
vmdk: Use g_random_int to generate CID
vmdk: Fix comment to match code of extent lines
vmdk: Clean up descriptor file reading
vmdk: Check descriptor file length when reading it
vmdk: Remove unnecessary initialization
vmdk: Set errp on failures in vmdk_open_vmdk4
block/vmdk.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
@ 2014-12-03 10:28 ` Fam Zheng
2014-12-03 14:11 ` Max Reitz
2014-12-03 17:06 ` Don Koch
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 2/6] vmdk: Fix comment to match code of extent lines Fam Zheng
` (4 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
This replaces two "time(NULL)" invocations with "g_random_int()".
According to VMDK spec, CID "is a random 32‐bit value updated the first
time the content of the virtual disk is modified after the virtual disk
is opened". Using "seconds since epoch" is just a "lame way" to generate
it, and not completely safe because of the low precision.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 2cbfd3e..ebb4b70 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -28,6 +28,7 @@
#include "qemu/module.h"
#include "migration/migration.h"
#include <zlib.h>
+#include <glib.h>
#define VMDK3_MAGIC (('C' << 24) | ('O' << 16) | ('W' << 8) | 'D')
#define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
@@ -1538,7 +1539,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
/* update CID on the first write every time the virtual disk is
* opened */
if (!s->cid_updated) {
- ret = vmdk_write_cid(bs, time(NULL));
+ ret = vmdk_write_cid(bs, g_random_int());
if (ret < 0) {
return ret;
}
@@ -1922,7 +1923,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
}
/* generate descriptor file */
desc = g_strdup_printf(desc_template,
- (uint32_t)time(NULL),
+ g_random_int(),
parent_cid,
fmt,
parent_desc_line,
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 2/6] vmdk: Fix comment to match code of extent lines
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID Fam Zheng
@ 2014-12-03 10:28 ` Fam Zheng
2014-12-03 14:13 ` Max Reitz
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading Fam Zheng
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
commit 04d542c8b (vmdk: support vmfs files) added support of VMFS extent
type but the comment above the changed code is left out. Update the
comment so they are consistent.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index ebb4b70..28d22db 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -785,10 +785,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
VmdkExtent *extent;
while (*p) {
- /* parse extent line:
+ /* parse extent line in one of below formats:
+ *
* RW [size in sectors] FLAT "file-name.vmdk" OFFSET
- * or
* RW [size in sectors] SPARSE "file-name.vmdk"
+ * RW [size in sectors] VMFS "file-name.vmdk"
*/
flat_offset = -1;
ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 2/6] vmdk: Fix comment to match code of extent lines Fam Zheng
@ 2014-12-03 10:28 ` Fam Zheng
2014-12-03 14:16 ` Max Reitz
` (2 more replies)
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it Fam Zheng
` (2 subsequent siblings)
5 siblings, 3 replies; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
Zeroing a buffer that will be filled right after is not necessary, and
allocating a power of two + 1 is naughty.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
block/vmdk.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 28d22db..82257cd 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -557,8 +557,8 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
return NULL;
}
- size = MIN(size, 1 << 20); /* avoid unbounded allocation */
- buf = g_malloc0(size + 1);
+ size = MIN(size, (1 << 20) - 1); /* avoid unbounded allocation */
+ buf = g_malloc(size + 1);
ret = bdrv_pread(file, desc_offset, buf, size);
if (ret < 0) {
@@ -566,6 +566,7 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
g_free(buf);
return NULL;
}
+ buf[ret] = 0;
return buf;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
` (2 preceding siblings ...)
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading Fam Zheng
@ 2014-12-03 10:28 ` Fam Zheng
2014-12-03 14:17 ` Max Reitz
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4 Fam Zheng
5 siblings, 1 reply; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
Since a too small file cannot be a valid VMDK image, and also since the
buffer's first 4 bytes will be unconditionally examined by
vmdk_open_sparse, let's error out the small file case to be clear.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index 82257cd..045fd7a 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -557,6 +557,11 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
return NULL;
}
+ if (size < 4) {
+ error_setg_errno(errp, -size, "File is too small, not a valid image");
+ return NULL;
+ }
+
size = MIN(size, (1 << 20) - 1); /* avoid unbounded allocation */
buf = g_malloc(size + 1);
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
` (3 preceding siblings ...)
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it Fam Zheng
@ 2014-12-03 10:28 ` Fam Zheng
2014-12-03 14:18 ` Max Reitz
2014-12-03 17:10 ` Don Koch
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4 Fam Zheng
5 siblings, 2 replies; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
It will be assigned to the return value of vmdk_read_desc.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 045fd7a..ed492bd 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -910,7 +910,7 @@ exit:
static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
- char *buf = NULL;
+ char *buf;
int ret;
BDRVVmdkState *s = bs->opaque;
uint32_t magic;
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
` (4 preceding siblings ...)
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization Fam Zheng
@ 2014-12-03 10:28 ` Fam Zheng
2014-12-03 14:20 ` Max Reitz
2014-12-03 17:14 ` Don Koch
5 siblings, 2 replies; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 10:28 UTC (permalink / raw
To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
block/vmdk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/block/vmdk.c b/block/vmdk.c
index ed492bd..127479d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -642,6 +642,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
bs->file->total_sectors * 512 - 1536,
&footer, sizeof(footer));
if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to read footer");
return ret;
}
@@ -653,6 +654,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
le32_to_cpu(footer.eos_marker.size) != 0 ||
le32_to_cpu(footer.eos_marker.type) != MARKER_END_OF_STREAM)
{
+ error_setg(errp, "Invalid footer");
return -EINVAL;
}
@@ -683,6 +685,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
* le64_to_cpu(header.granularity);
if (l1_entry_sectors == 0) {
+ error_setg(errp, "L1 entry size is invalid");
return -EINVAL;
}
l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
--
1.9.3
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID Fam Zheng
@ 2014-12-03 14:11 ` Max Reitz
2014-12-03 17:06 ` Don Koch
1 sibling, 0 replies; 19+ messages in thread
From: Max Reitz @ 2014-12-03 14:11 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
On 2014-12-03 at 11:28, Fam Zheng wrote:
> This replaces two "time(NULL)" invocations with "g_random_int()".
> According to VMDK spec, CID "is a random 32‐bit value updated the first
> time the content of the virtual disk is modified after the virtual disk
> is opened". Using "seconds since epoch" is just a "lame way" to generate
> it, and not completely safe because of the low precision.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..ebb4b70 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -28,6 +28,7 @@
> #include "qemu/module.h"
> #include "migration/migration.h"
> #include <zlib.h>
> +#include <glib.h>
This should be covered by qemu-common.h already. But anyway:
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/6] vmdk: Fix comment to match code of extent lines
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 2/6] vmdk: Fix comment to match code of extent lines Fam Zheng
@ 2014-12-03 14:13 ` Max Reitz
0 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2014-12-03 14:13 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
On 2014-12-03 at 11:28, Fam Zheng wrote:
> commit 04d542c8b (vmdk: support vmfs files) added support of VMFS extent
> type but the comment above the changed code is left out. Update the
> comment so they are consistent.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index ebb4b70..28d22db 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -785,10 +785,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
> VmdkExtent *extent;
>
> while (*p) {
> - /* parse extent line:
> + /* parse extent line in one of below formats:
> + *
> * RW [size in sectors] FLAT "file-name.vmdk" OFFSET
> - * or
> * RW [size in sectors] SPARSE "file-name.vmdk"
> + * RW [size in sectors] VMFS "file-name.vmdk"
> */
> flat_offset = -1;
> ret = sscanf(p, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64,
Isn't VMFSSPARSE missing? It doesn't seem to be handled any different
than SPARSE, but then again, VMFS isn't handled so differently than FLAT
either (except it doesn't take an offset)...
Max
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading Fam Zheng
@ 2014-12-03 14:16 ` Max Reitz
2014-12-03 14:32 ` Markus Armbruster
2014-12-03 17:09 ` Don Koch
2 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2014-12-03 14:16 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
On 2014-12-03 at 11:28, Fam Zheng wrote:
> Zeroing a buffer that will be filled right after is not necessary, and
> allocating a power of two + 1 is naughty.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
> block/vmdk.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it Fam Zheng
@ 2014-12-03 14:17 ` Max Reitz
2014-12-03 23:13 ` Fam Zheng
0 siblings, 1 reply; 19+ messages in thread
From: Max Reitz @ 2014-12-03 14:17 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
On 2014-12-03 at 11:28, Fam Zheng wrote:
> Since a too small file cannot be a valid VMDK image, and also since the
> buffer's first 4 bytes will be unconditionally examined by
> vmdk_open_sparse, let's error out the small file case to be clear.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 82257cd..045fd7a 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -557,6 +557,11 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
> return NULL;
> }
>
> + if (size < 4) {
> + error_setg_errno(errp, -size, "File is too small, not a valid image");
0 <= size < 4, strerror(0..-3) doesn't make a whole lot of sense. Should
be simply error_setg() instead of error_setg_errno().
Max
> + return NULL;
> + }
> +
> size = MIN(size, (1 << 20) - 1); /* avoid unbounded allocation */
> buf = g_malloc(size + 1);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization Fam Zheng
@ 2014-12-03 14:18 ` Max Reitz
2014-12-03 17:10 ` Don Koch
1 sibling, 0 replies; 19+ messages in thread
From: Max Reitz @ 2014-12-03 14:18 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
On 2014-12-03 at 11:28, Fam Zheng wrote:
> It will be assigned to the return value of vmdk_read_desc.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4 Fam Zheng
@ 2014-12-03 14:20 ` Max Reitz
2014-12-03 17:14 ` Don Koch
1 sibling, 0 replies; 19+ messages in thread
From: Max Reitz @ 2014-12-03 14:20 UTC (permalink / raw
To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Stefan Hajnoczi
On 2014-12-03 at 11:28, Fam Zheng wrote:
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/vmdk.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Max Reitz <mreitz@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading Fam Zheng
2014-12-03 14:16 ` Max Reitz
@ 2014-12-03 14:32 ` Markus Armbruster
2014-12-03 17:09 ` Don Koch
2 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2014-12-03 14:32 UTC (permalink / raw
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
Fam Zheng <famz@redhat.com> writes:
> Zeroing a buffer that will be filled right after is not necessary, and
> allocating a power of two + 1 is naughty.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID Fam Zheng
2014-12-03 14:11 ` Max Reitz
@ 2014-12-03 17:06 ` Don Koch
1 sibling, 0 replies; 19+ messages in thread
From: Don Koch @ 2014-12-03 17:06 UTC (permalink / raw
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Markus Armbruster
On Wed, 3 Dec 2014 18:28:36 +0800
Fam Zheng <famz@redhat.com> wrote:
> This replaces two "time(NULL)" invocations with "g_random_int()".
> According to VMDK spec, CID "is a random 32‐bit value updated the first
> time the content of the virtual disk is modified after the virtual disk
> is opened". Using "seconds since epoch" is just a "lame way" to generate
> it, and not completely safe because of the low precision.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Don Koch <dkoch@verizon.com>
> block/vmdk.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..ebb4b70 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -28,6 +28,7 @@
> #include "qemu/module.h"
> #include "migration/migration.h"
> #include <zlib.h>
> +#include <glib.h>
>
> #define VMDK3_MAGIC (('C' << 24) | ('O' << 16) | ('W' << 8) | 'D')
> #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
> @@ -1538,7 +1539,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num,
> /* update CID on the first write every time the virtual disk is
> * opened */
> if (!s->cid_updated) {
> - ret = vmdk_write_cid(bs, time(NULL));
> + ret = vmdk_write_cid(bs, g_random_int());
> if (ret < 0) {
> return ret;
> }
> @@ -1922,7 +1923,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
> }
> /* generate descriptor file */
> desc = g_strdup_printf(desc_template,
> - (uint32_t)time(NULL),
> + g_random_int(),
> parent_cid,
> fmt,
> parent_desc_line,
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading Fam Zheng
2014-12-03 14:16 ` Max Reitz
2014-12-03 14:32 ` Markus Armbruster
@ 2014-12-03 17:09 ` Don Koch
2 siblings, 0 replies; 19+ messages in thread
From: Don Koch @ 2014-12-03 17:09 UTC (permalink / raw
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Markus Armbruster
On Wed, 3 Dec 2014 18:28:38 +0800
Fam Zheng <famz@redhat.com> wrote:
> Zeroing a buffer that will be filled right after is not necessary, and
> allocating a power of two + 1 is naughty.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
Reviewed-by: Don Koch <dkoch@verizon.com>
> block/vmdk.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 28d22db..82257cd 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -557,8 +557,8 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
> return NULL;
> }
>
> - size = MIN(size, 1 << 20); /* avoid unbounded allocation */
> - buf = g_malloc0(size + 1);
> + size = MIN(size, (1 << 20) - 1); /* avoid unbounded allocation */
> + buf = g_malloc(size + 1);
>
> ret = bdrv_pread(file, desc_offset, buf, size);
> if (ret < 0) {
> @@ -566,6 +566,7 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
> g_free(buf);
> return NULL;
> }
> + buf[ret] = 0;
>
> return buf;
> }
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization Fam Zheng
2014-12-03 14:18 ` Max Reitz
@ 2014-12-03 17:10 ` Don Koch
1 sibling, 0 replies; 19+ messages in thread
From: Don Koch @ 2014-12-03 17:10 UTC (permalink / raw
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Markus Armbruster
On Wed, 3 Dec 2014 18:28:40 +0800
Fam Zheng <famz@redhat.com> wrote:
> It will be assigned to the return value of vmdk_read_desc.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Don Koch <dkoch@verizon.com>
> block/vmdk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 045fd7a..ed492bd 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -910,7 +910,7 @@ exit:
> static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
> Error **errp)
> {
> - char *buf = NULL;
> + char *buf;
> int ret;
> BDRVVmdkState *s = bs->opaque;
> uint32_t magic;
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4 Fam Zheng
2014-12-03 14:20 ` Max Reitz
@ 2014-12-03 17:14 ` Don Koch
1 sibling, 0 replies; 19+ messages in thread
From: Don Koch @ 2014-12-03 17:14 UTC (permalink / raw
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Markus Armbruster
On Wed, 3 Dec 2014 18:28:41 +0800
Fam Zheng <famz@redhat.com> wrote:
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
Reviewed-by: Don Koch <dkoch@verizon.com>
> block/vmdk.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/block/vmdk.c b/block/vmdk.c
> index ed492bd..127479d 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -642,6 +642,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> bs->file->total_sectors * 512 - 1536,
> &footer, sizeof(footer));
> if (ret < 0) {
> + error_setg_errno(errp, -ret, "Failed to read footer");
> return ret;
> }
>
> @@ -653,6 +654,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> le32_to_cpu(footer.eos_marker.size) != 0 ||
> le32_to_cpu(footer.eos_marker.type) != MARKER_END_OF_STREAM)
> {
> + error_setg(errp, "Invalid footer");
> return -EINVAL;
> }
>
> @@ -683,6 +685,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
> l1_entry_sectors = le32_to_cpu(header.num_gtes_per_gt)
> * le64_to_cpu(header.granularity);
> if (l1_entry_sectors == 0) {
> + error_setg(errp, "L1 entry size is invalid");
> return -EINVAL;
> }
> l1_size = (le64_to_cpu(header.capacity) + l1_entry_sectors - 1)
> --
> 1.9.3
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it
2014-12-03 14:17 ` Max Reitz
@ 2014-12-03 23:13 ` Fam Zheng
0 siblings, 0 replies; 19+ messages in thread
From: Fam Zheng @ 2014-12-03 23:13 UTC (permalink / raw
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Markus Armbruster
On Wed, 12/03 15:17, Max Reitz wrote:
> On 2014-12-03 at 11:28, Fam Zheng wrote:
> >Since a too small file cannot be a valid VMDK image, and also since the
> >buffer's first 4 bytes will be unconditionally examined by
> >vmdk_open_sparse, let's error out the small file case to be clear.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >---
> > block/vmdk.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> >diff --git a/block/vmdk.c b/block/vmdk.c
> >index 82257cd..045fd7a 100644
> >--- a/block/vmdk.c
> >+++ b/block/vmdk.c
> >@@ -557,6 +557,11 @@ static char *vmdk_read_desc(BlockDriverState *file, uint64_t desc_offset,
> > return NULL;
> > }
> >+ if (size < 4) {
> >+ error_setg_errno(errp, -size, "File is too small, not a valid image");
>
> 0 <= size < 4, strerror(0..-3) doesn't make a whole lot of sense. Should be
> simply error_setg() instead of error_setg_errno().
Yes, fixing.
Fam
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2014-12-03 23:13 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-03 10:28 [Qemu-devel] [PATCH v3 0/6] vmdk: A few small fixes Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 1/6] vmdk: Use g_random_int to generate CID Fam Zheng
2014-12-03 14:11 ` Max Reitz
2014-12-03 17:06 ` Don Koch
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 2/6] vmdk: Fix comment to match code of extent lines Fam Zheng
2014-12-03 14:13 ` Max Reitz
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 3/6] vmdk: Clean up descriptor file reading Fam Zheng
2014-12-03 14:16 ` Max Reitz
2014-12-03 14:32 ` Markus Armbruster
2014-12-03 17:09 ` Don Koch
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 4/6] vmdk: Check descriptor file length when reading it Fam Zheng
2014-12-03 14:17 ` Max Reitz
2014-12-03 23:13 ` Fam Zheng
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 5/6] vmdk: Remove unnecessary initialization Fam Zheng
2014-12-03 14:18 ` Max Reitz
2014-12-03 17:10 ` Don Koch
2014-12-03 10:28 ` [Qemu-devel] [PATCH v3 6/6] vmdk: Set errp on failures in vmdk_open_vmdk4 Fam Zheng
2014-12-03 14:20 ` Max Reitz
2014-12-03 17:14 ` Don Koch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.