Re: [Qemu-devel] [PATCH] handle_aiocb_rw() can't distinguish betweenan error and 0 bytes transferred
Hi,
Thank you so much for your reply. Yes, I cannot use gmail directly for it's
blocked by the GFW, and I used the Tencent mail(@qq.com) to receive mails in
gmail. It's my first time to send a patch, I will read the link you provide
and improve my future patches.
As for BOOL and bool, are some other Windows types such as DWORD not preferred
to standard C types?
I will send a v3 patch based on your fix after a week for I'll travel for a
week in coming National Day holiday.
Thanks again for you patient.
Sincerely.
Guangmu Zhu
Hi,
thanks for sending this patch. The actual change looks correct, but
there are a few formal problems with your patch submission.
I can fix these problems myself and apply the patch anyway, but I want
to let you know so you can improve your future patches. If you were not
a first time contributor, these would be reasons to just reject the
patch, because they make maintainers' lives hard.
The first thing I want to mention is the following wiki page:
http://wiki.qemu.org/Contribute/SubmitAPatch
If there is any way for you to do so, please use git-send-email to send
your patches. If you can't, absolutely avoid HTML or multipart emails. I
have macros to make applying patches from the list easy for me (using
'git am'); but they only work properly with plain text emails.
Am 26.09.2015 um 03:54 hat Guangmu Zhu geschrieben:
> Correct patch format.
>
> Signed-off-by: Guangmu Zhu <guangmu...@gmail.com>
If you send an improved version of a patch, please make sure to include
a new version number in the subject line, as in "[PATCH v2]". You get
this with 'git-format-patch -v2 ...'
Also keep a commit message that describes the change that the patch
makes. Describe the change between v1 and v2 (i.e. "Correct patch
format.") below the "---" line, so that 'git am' drops them when
applying. Such comments are useful for review on the mailing list, but
not in the commit log.
> ---
> block/raw-win32.c | 48 ++--
> 1 file changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 68f2338..569dda2 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -60,11 +60,13 @@ typedef struct BDRVRawState {
> * Returns the number of bytes handles or -errno in case of an error. Short
> * reads are only returned if the end of the file is reached.
> */
> -static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
> +static size_t handle_aiocb_rw(RawWin32AIOData *aiocb, BOOL *err)
I would prefer standard C bool to Windows's BOOL.
In fact, an even better interface might be returning ssize_t and
returning -EINVAL in error cases, so that the caller doesn't have to do
that conversion any more.
> {
> size_t offset = 0;
> int i;
>
> +*err = FALSE;
> +
> for (i = 0; i < aiocb->aio_niov; i++) {
> OVERLAPPED ov;
> DWORD ret, ret_count, len;
> @@ -81,7 +83,8 @@ static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
> len, _count, );
> }
> if (!ret) {
> -ret_count = 0;
> +*err = TRUE;
> +break;
> }
> if (ret_count != len) {
> offset += ret_count;
> @@ -98,30 +101,39 @@ static int aio_worker(void *arg)
> RawWin32AIOData *aiocb = arg;
> ssize_t ret = 0;
> size_t count;
> +BOOL err = FALSE;
>
> switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
> case QEMU_AIO_READ:
> -count = handle_aiocb_rw(aiocb);
> -if (count < aiocb->aio_nbytes) {
> -/* A short read means that we have reached EOF. Pad the buffer
> - * with zeros for bytes after EOF. */
> -iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
> - 0, aiocb->aio_nbytes - count);
> -
> -count = aiocb->aio_nbytes;
> -}
> -if (count == aiocb->aio_nbytes) {
> -ret = 0;
> -} else {
> +count = handle_aiocb_rw(aiocb, );
> +if (err) {
> ret = -EINVAL;
> +} else {
> +if (count < aiocb->aio_nbytes) {
> +/* A short read means that we have reached EOF. Pad the
> buffer
> + * with zeros for bytes after EOF. */
> +iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
> + 0, aiocb->aio_nbytes - count);
> +
> +count = aiocb->aio_nbytes;
> +}
> +if (count == aiocb->aio_nbytes) {
> +ret = 0;
> +} else {
> +ret = -EINVAL;
> +
Re: [Qemu-devel] [PATCH] No FreeWriterMetadata() after GatherWriterMetadata() in requester.cpp
I sent the patch some days ago and received no response. I have no idea if this
means the patch is not Ok or just no one had interest in it...
Sincerely.
Guangmu Zhu
--
FreeWriterMetadata() should be called if GatherWriterMetadata() succeeded.
Signed-off-by: Guangmu Zhu <guangmu...@gmail.com>
---
qga/vss-win32/requester.cpp | 11 +++
1 file changed, 11 insertions(+)
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 9b3e310..337f722 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -47,6 +47,7 @@ static struct QGAVSSContext {
HANDLE hEventFrozen; /* notify fs/writer freeze from provider */
HANDLE hEventThaw; /* request provider to thaw */
HANDLE hEventTimeout; /* notify timeout in provider */
+BOOL bWriterMetadataGathered; /* TRUE if GatherWriterMetadata succeed */
int cFrozenVols; /* number of frozen volumes */
} vss_ctx;
@@ -92,6 +93,8 @@ STDAPI requester_init(void)
static void requester_cleanup(void)
{
+HRESULT hr = S_OK;
+
if (vss_ctx.hEventFrozen) {
CloseHandle(vss_ctx.hEventFrozen);
vss_ctx.hEventFrozen = NULL;
@@ -108,6 +111,12 @@ static void requester_cleanup(void)
vss_ctx.pAsyncSnapshot->Release();
vss_ctx.pAsyncSnapshot = NULL;
}
+if (vss_ctx.bWriterMetadataGathered) {
+hr = vss_ctx.pVssbc->FreeWriterMetadata();
+if (FAILED(hr)) {
+err_set(errset, hr, "failed to free writer metadata");
+}
+}
if (vss_ctx.pVssbc) {
vss_ctx.pVssbc->Release();
vss_ctx.pVssbc = NULL;
@@ -323,8 +332,10 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
}
if (FAILED(hr)) {
err_set(errset, hr, "failed to gather writer metadata");
+vss_ctx.bWriterMetadataGathered = FALSE;
goto out;
}
+vss_ctx.bWriterMetadataGathered = TRUE;
AddComponents(errset);
if (err_is_set(errset)) {
--
2.1.4
[Qemu-devel] [PATCH] handle_aiocb_rw() can't distinguish between an error and 0 bytes transferred
The handle_aiocb_rw() can't distinguish between an error and 0 bytes
transferred.
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..569dda2 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -60,11 +60,13 @@ typedef struct BDRVRawState {
* Returns the number of bytes handles or -errno in case of an error. Short
* reads are only returned if the end of the file is reached.
*/
-static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
+static size_t handle_aiocb_rw(RawWin32AIOData *aiocb, BOOL *err)
{
size_t offset = 0;
int i;
+*err = FALSE;
+
for (i = 0; i < aiocb->aio_niov; i++) {
OVERLAPPED ov;
DWORD ret, ret_count, len;
@@ -81,7 +83,8 @@ static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
len, _count, );
}
if (!ret) {
-ret_count = 0;
+*err = TRUE;
+break;
}
if (ret_count != len) {
offset += ret_count;
@@ -98,30 +101,39 @@ static int aio_worker(void *arg)
RawWin32AIOData *aiocb = arg;
ssize_t ret = 0;
size_t count;
+BOOL err = FALSE;
switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
case QEMU_AIO_READ:
-count = handle_aiocb_rw(aiocb);
-if (count < aiocb->aio_nbytes) {
-/* A short read means that we have reached EOF. Pad the buffer
- * with zeros for bytes after EOF. */
-iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
- 0, aiocb->aio_nbytes - count);
-
-count = aiocb->aio_nbytes;
-}
-if (count == aiocb->aio_nbytes) {
-ret = 0;
-} else {
+count = handle_aiocb_rw(aiocb, );
+if (err) {
ret = -EINVAL;
+} else {
+if (count < aiocb->aio_nbytes) {
+/* A short read means that we have reached EOF. Pad the buffer
+ * with zeros for bytes after EOF. */
+iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
+ 0, aiocb->aio_nbytes - count);
+
+count = aiocb->aio_nbytes;
+}
+if (count == aiocb->aio_nbytes) {
+ret = 0;
+} else {
+ret = -EINVAL;
+}
}
break;
case QEMU_AIO_WRITE:
-count = handle_aiocb_rw(aiocb);
-if (count == aiocb->aio_nbytes) {
-count = 0;
+count = handle_aiocb_rw(aiocb, );
+if (err) {
+ret = -EINVAL;
} else {
-count = -EINVAL;
+if (count == aiocb->aio_nbytes) {
+count = 0;
+} else {
+count = -EINVAL;
+}
}
break;
case QEMU_AIO_FLUSH:
Re: [Qemu-devel] [PATCH] handle_aiocb_rw() can't distinguish between an error and 0 bytes transferred
Correct patch format.
Signed-off-by: Guangmu Zhu <guangmu...@gmail.com>
---
block/raw-win32.c | 48 ++--
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..569dda2 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -60,11 +60,13 @@ typedef struct BDRVRawState {
* Returns the number of bytes handles or -errno in case of an error. Short
* reads are only returned if the end of the file is reached.
*/
-static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
+static size_t handle_aiocb_rw(RawWin32AIOData *aiocb, BOOL *err)
{
size_t offset = 0;
int i;
+*err = FALSE;
+
for (i = 0; i < aiocb->aio_niov; i++) {
OVERLAPPED ov;
DWORD ret, ret_count, len;
@@ -81,7 +83,8 @@ static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
len, _count, );
}
if (!ret) {
-ret_count = 0;
+*err = TRUE;
+break;
}
if (ret_count != len) {
offset += ret_count;
@@ -98,30 +101,39 @@ static int aio_worker(void *arg)
RawWin32AIOData *aiocb = arg;
ssize_t ret = 0;
size_t count;
+BOOL err = FALSE;
switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
case QEMU_AIO_READ:
-count = handle_aiocb_rw(aiocb);
-if (count < aiocb->aio_nbytes) {
-/* A short read means that we have reached EOF. Pad the buffer
- * with zeros for bytes after EOF. */
-iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
- 0, aiocb->aio_nbytes - count);
-
-count = aiocb->aio_nbytes;
-}
-if (count == aiocb->aio_nbytes) {
-ret = 0;
-} else {
+count = handle_aiocb_rw(aiocb, );
+if (err) {
ret = -EINVAL;
+} else {
+if (count < aiocb->aio_nbytes) {
+/* A short read means that we have reached EOF. Pad the buffer
+ * with zeros for bytes after EOF. */
+iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
+ 0, aiocb->aio_nbytes - count);
+
+count = aiocb->aio_nbytes;
+}
+if (count == aiocb->aio_nbytes) {
+ret = 0;
+} else {
+ret = -EINVAL;
+}
}
break;
case QEMU_AIO_WRITE:
-count = handle_aiocb_rw(aiocb);
-if (count == aiocb->aio_nbytes) {
-count = 0;
+count = handle_aiocb_rw(aiocb, );
+if (err) {
+ret = -EINVAL;
} else {
-count = -EINVAL;
+if (count == aiocb->aio_nbytes) {
+count = 0;
+} else {
+count = -EINVAL;
+}
}
break;
case QEMU_AIO_FLUSH:
--
2.1.4
-
The handle_aiocb_rw() can't distinguish between an error and 0 bytes
transferred.
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..569dda2 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -60,11 +60,13 @@ typedef struct BDRVRawState {
* Returns the number of bytes handles or -errno in case of an error. Short
* reads are only returned if the end of the file is reached.
*/
-static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
+static size_t handle_aiocb_rw(RawWin32AIOData *aiocb, BOOL *err)
{
size_t offset = 0;
int i;
+*err = FALSE;
+
for (i = 0; i < aiocb->aio_niov; i++) {
OVERLAPPED ov;
DWORD ret, ret_count, len;
@@ -81,7 +83,8 @@ static size_t handle_aiocb_rw(RawWin32AIOData *aiocb)
len, _count, );
}
if (!ret) {
-ret_count = 0;
+*err = TRUE;
+break;
}
if (ret_count != len) {
offset += ret_count;
@@ -98,30 +101,39 @@ static int aio_worker(void *arg)
RawWin32AIOData *aiocb = arg;
ssize_t ret = 0;
size_t count;
+BOOL err = FALSE;
switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
case QEMU_AIO_READ:
-count = handle_aiocb_rw(aiocb);
-if (count < aiocb->aio_nbytes) {
-/* A short read means that we have reached EOF. Pad the buffer
- * with zeros for bytes after EOF. */
-iov_memset(aiocb->aio_iov, aiocb->aio_niov, count,
- 0, aiocb->aio_nbytes - count);
-
-count = aiocb->aio_nbytes;
-}
-if (count == aiocb->aio_nbytes) {
-ret = 0;
-} else {
+count = handle_aiocb_rw(aiocb, );
+if (err) {
ret = -EINVAL;
+} else {
+if (count < aiocb->aio_nbytes) {
+/* A short read means that we have rea
Re: [Qemu-devel] [PATCH] No FreeWriterMetadata() after GatherWriterMetadata() in requester.cpp
Correct patch format.
Signed-off-by: Guangmu Zhu <guangmu...@gmail.com>
---
qga/vss-win32/requester.cpp | 11 +++
1 file changed, 11 insertions(+)
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 9b3e310..337f722 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -47,6 +47,7 @@ static struct QGAVSSContext {
HANDLE hEventFrozen; /* notify fs/writer freeze from provider */
HANDLE hEventThaw; /* request provider to thaw */
HANDLE hEventTimeout; /* notify timeout in provider */
+BOOL bWriterMetadataGathered; /* TRUE if GatherWriterMetadata succeed */
int cFrozenVols; /* number of frozen volumes */
} vss_ctx;
@@ -92,6 +93,8 @@ STDAPI requester_init(void)
static void requester_cleanup(void)
{
+HRESULT hr = S_OK;
+
if (vss_ctx.hEventFrozen) {
CloseHandle(vss_ctx.hEventFrozen);
vss_ctx.hEventFrozen = NULL;
@@ -108,6 +111,12 @@ static void requester_cleanup(void)
vss_ctx.pAsyncSnapshot->Release();
vss_ctx.pAsyncSnapshot = NULL;
}
+if (vss_ctx.bWriterMetadataGathered) {
+hr = vss_ctx.pVssbc->FreeWriterMetadata();
+if (FAILED(hr)) {
+err_set(errset, hr, "failed to free writer metadata");
+}
+}
if (vss_ctx.pVssbc) {
vss_ctx.pVssbc->Release();
vss_ctx.pVssbc = NULL;
@@ -323,8 +332,10 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
}
if (FAILED(hr)) {
err_set(errset, hr, "failed to gather writer metadata");
+vss_ctx.bWriterMetadataGathered = FALSE;
goto out;
}
+vss_ctx.bWriterMetadataGathered = TRUE;
AddComponents(errset);
if (err_is_set(errset)) {
--
2.1.4
FreeWriterMetadata() should be called if GatherWriterMetadata() succeeded.
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 9b3e310..337f722 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -47,6 +47,7 @@ static struct QGAVSSContext {
HANDLE hEventFrozen; /* notify fs/writer freeze from provider */
HANDLE hEventThaw; /* request provider to thaw */
HANDLE hEventTimeout; /* notify timeout in provider */
+BOOL bWriterMetadataGathered; /* TRUE if GatherWriterMetadata succeed */
int cFrozenVols; /* number of frozen volumes */
} vss_ctx;
@@ -92,6 +93,8 @@ STDAPI requester_init(void)
static void requester_cleanup(void)
{
+HRESULT hr = S_OK;
+
if (vss_ctx.hEventFrozen) {
CloseHandle(vss_ctx.hEventFrozen);
vss_ctx.hEventFrozen = NULL;
@@ -108,6 +111,12 @@ static void requester_cleanup(void)
vss_ctx.pAsyncSnapshot->Release();
vss_ctx.pAsyncSnapshot = NULL;
}
+if (vss_ctx.bWriterMetadataGathered) {
+hr = vss_ctx.pVssbc->FreeWriterMetadata();
+if (FAILED(hr)) {
+err_set(errset, hr, "failed to free writer metadata");
+}
+}
if (vss_ctx.pVssbc) {
vss_ctx.pVssbc->Release();
vss_ctx.pVssbc = NULL;
@@ -323,8 +332,10 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
}
if (FAILED(hr)) {
err_set(errset, hr, "failed to gather writer metadata");
+vss_ctx.bWriterMetadataGathered = FALSE;
goto out;
}
+vss_ctx.bWriterMetadataGathered = TRUE;
AddComponents(errset);
if (err_is_set(errset)) {
[Qemu-devel] [PATCH] No FreeWriterMetadata() after GatherWriterMetadata() in requester.cpp
FreeWriterMetadata() should be called if GatherWriterMetadata() succeeded.
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 9b3e310..337f722 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -47,6 +47,7 @@ static struct QGAVSSContext {
HANDLE hEventFrozen; /* notify fs/writer freeze from provider */
HANDLE hEventThaw; /* request provider to thaw */
HANDLE hEventTimeout; /* notify timeout in provider */
+BOOL bWriterMetadataGathered; /* TRUE if GatherWriterMetadata succeed */
int cFrozenVols; /* number of frozen volumes */
} vss_ctx;
@@ -92,6 +93,8 @@ STDAPI requester_init(void)
static void requester_cleanup(void)
{
+HRESULT hr = S_OK;
+
if (vss_ctx.hEventFrozen) {
CloseHandle(vss_ctx.hEventFrozen);
vss_ctx.hEventFrozen = NULL;
@@ -108,6 +111,12 @@ static void requester_cleanup(void)
vss_ctx.pAsyncSnapshot->Release();
vss_ctx.pAsyncSnapshot = NULL;
}
+if (vss_ctx.bWriterMetadataGathered) {
+hr = vss_ctx.pVssbc->FreeWriterMetadata();
+if (FAILED(hr)) {
+err_set(errset, hr, "failed to free writer metadata");
+}
+}
if (vss_ctx.pVssbc) {
vss_ctx.pVssbc->Release();
vss_ctx.pVssbc = NULL;
@@ -323,8 +332,10 @@ void requester_freeze(int *num_vols, ErrorSet *errset)
}
if (FAILED(hr)) {
err_set(errset, hr, "failed to gather writer metadata");
+vss_ctx.bWriterMetadataGathered = FALSE;
goto out;
}
+vss_ctx.bWriterMetadataGathered = TRUE;
AddComponents(errset);
if (err_is_set(errset)) {
Re: [Qemu-devel] No error report when using the qemu-img.exetoconvert a disk to vmdk format which is saved on a disk that has nomorespace
Hi Kevin,
I tried the patch you provide, and I haven't seen that problem yet. If the disk
space is full, an error will be reported with the message "Invalid argument"
and the program will stop.
Will you merge the patch to the master?
Thanks.
Guangmu Zhu
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
case QEMU_AIO_WRITE:
count = handle_aiocb_rw(aiocb);
if (count == aiocb->aio_nbytes) {
-count = 0;
+ret = 0;
} else {
-count = -EINVAL;
+ret = -EINVAL;
}
break;
case QEMU_AIO_FLUSH:
-
I'll try the patch and report in a week for I'm too busy these days. And if I
could, I would like to help to maintain the Windows backend.
Sincerely.
Guangmu Zhu
-
Am 23.09.2015 um 13:30 hat Guangmu Zhu geschrieben:
> If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
> called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite
> (extent->file, write_offset, write_buf, write_len);". So the "extend->file" is
> "bdrv_file", is it?
Yes, exactly. You'll go through bdrv_vmdk first, and then the nested
call goes to bdrv_file.
> -
>
> Correct a mistake:
> So though the "count" would be "-EINVAL" if error occurred while writing some
> file, the return value will always be zero. Maybe I missed something?
I think you're right. Instead of setting count = 0/-EINVAL in
aio_worker, we should be setting ret.
Can you try the patch below and report back?
> 3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
> "raw-win32.c" and the quemu-img uses synchronous IO always, so the function
> "paio_submit" in the same file will be called. This function submits the "aio"
> to "worker_thread" with the callback "aio_worker". There are some codes in
> "aio_worker":
>
> ssize_t ret = 0;
> ..
> case QEMU_AIO_WRITE:
> count = handle_aiocb_rw(aiocb);
> if (count == aiocb->aio_nbytes) {
> count = 0;
> } else {
> count = -EINVAL;
> }
> break;
> ..
> return ret;
Independently of your problem, the code in aio_worker() looks a bit
fishy, because handle_aiocb_rw() can't distinguish between an error
and 0 bytes transferred.
For writes, that probably doesn't matter, but for reads, I think we
return a successful read of zeroes instead of signalling an error. This
might need another patch.
Generally, the Windows backend is not getting a lot of attention and
could use someone who checks it, cleans it up and fixes bugs.
Kevin
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
case QEMU_AIO_WRITE:
count = handle_aiocb_rw(aiocb);
if (count == aiocb->aio_nbytes) {
-count = 0;
+ret = 0;
} else {
-count = -EINVAL;
+ret = -EINVAL;
}
break;
case QEMU_AIO_FLUSH:
Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace
Thanks for your reply.
I read the source code again and have some question:
1. qume-img wrote the target file in the function "bdrv_aligned_pwritev" which
called the "drv->bdrv_co_writev" function. I haven't know how the qume-img
found the right driver for "drv", but I guessed it's "BlockDriver bdrv_file"
for the file on Windows, is it?
2. If the above is correct, the member "bdrv_aio_writev" of "BlockDriver" is
the function "raw_aio_writev" while the member "bdrv_co_writev" is NULL. The
function "bdrv_register" registers the "BlockDriver" and checks whether the
"bdrv_co_readv" or "bdrv_aio_readv" is NULL. If the "bdrv_co_readv" is NULL,
the member "bdrv_co_writev" is "bdrv_co_writev_em" which calls the function
"bdrv_co_io_em". There are some codes of it:
static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *iov,
bool is_write)
{
CoroutineIOCompletion co = {
.coroutine = qemu_coroutine_self(),
};
BlockAIOCB *acb;
if (is_write) {
acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
bdrv_co_io_em_complete, );
} else {
acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
bdrv_co_io_em_complete, );
}
trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
if (!acb) {
return -EIO;
}
qemu_coroutine_yield();
return co.ret;
}
3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
"raw-win32.c" and the quemu-img uses synchronous IO always, so the function
"paio_submit" in the same file will be called. This function submits the "aio"
to "worker_thread" with the callback "aio_worker". There are some codes in
"aio_worker":
ssize_t ret = 0;
..
case QEMU_AIO_WRITE:
count = handle_aiocb_rw(aiocb);
if (count == aiocb->aio_nbytes) {
count = 0;
} else {
count = -EINVAL;
}
break;
..
return ret;
So though the "count" would be zero if error occurred while writing some file,
the return value will always be zero. Maybe I missed something?
4. The function "worker_thread" calls the callback:
ret = req->func(req->arg);
req->ret = ret;
/* Write ret before state. */
smp_wmb();
req->state = THREAD_DONE;
qemu_mutex_lock(>lock);
qemu_bh_schedule(pool->completion_bh);
The "pool->completion_bh" is function "thread_pool_completion_bh", which calls
"elem->common.cb(elem->common.opaque, elem->ret);". And the "elem->common.cb"
is function "bdrv_co_io_em_complete":
static void bdrv_co_io_em_complete(void *opaque, int ret)
{
CoroutineIOCompletion *co = opaque;
co->ret = ret;
qemu_coroutine_enter(co->coroutine, NULL);
}
5. Finally the return value(zero) will be stored in "co.ret" in function
"bdrv_co_io_em". However, what would happen if the req haven't done but the
function "bdrv_co_io_em" returned? The write operation would return an
uninitialized value(is it zero? I don't know.), is it?
Maybe these made the program report nothing with writer errors, I think.
Thanks again in advance.
Guangmu Zhu
-
Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
>
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
> which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
> "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
> option to change the default behavior of the error report.
>
> So I think if there were some ways to change the default value of the error
> report, it might be better. Further more, I suggest we could just add some
> codes to the "img_convert" function:
>
> 1827:out_blk = img_open("target", out_filename, out_fmt, flags,
> true,
> quiet);
> 1828
Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace
Correct a mistake:
So though the "count" would be "-EINVAL" if error occurred while writing some
file, the return value will always be zero. Maybe I missed something?
Sorry.
Guangmu Zhu
-
Thanks for your reply.
I read the source code again and have some question:
1. qume-img wrote the target file in the function "bdrv_aligned_pwritev" which
called the "drv->bdrv_co_writev" function. I haven't know how the qume-img
found the right driver for "drv", but I guessed it's "BlockDriver bdrv_file"
for the file on Windows, is it?
2. If the above is correct, the member "bdrv_aio_writev" of "BlockDriver" is
the function "raw_aio_writev" while the member "bdrv_co_writev" is NULL. The
function "bdrv_register" registers the "BlockDriver" and checks whether the
"bdrv_co_readv" or "bdrv_aio_readv" is NULL. If the "bdrv_co_readv" is NULL,
the member "bdrv_co_writev" is "bdrv_co_writev_em" which calls the function
"bdrv_co_io_em". There are some codes of it:
static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *iov,
bool is_write)
{
CoroutineIOCompletion co = {
.coroutine = qemu_coroutine_self(),
};
BlockAIOCB *acb;
if (is_write) {
acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
bdrv_co_io_em_complete, );
} else {
acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
bdrv_co_io_em_complete, );
}
trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
if (!acb) {
return -EIO;
}
qemu_coroutine_yield();
return co.ret;
}
3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
"raw-win32.c" and the quemu-img uses synchronous IO always, so the function
"paio_submit" in the same file will be called. This function submits the "aio"
to "worker_thread" with the callback "aio_worker". There are some codes in
"aio_worker":
ssize_t ret = 0;
..
case QEMU_AIO_WRITE:
count = handle_aiocb_rw(aiocb);
if (count == aiocb->aio_nbytes) {
count = 0;
} else {
count = -EINVAL;
}
break;
..
return ret;
So though the "count" would be zero if error occurred while writing some file,
the return value will always be zero. Maybe I missed something?
4. The function "worker_thread" calls the callback:
ret = req->func(req->arg);
req->ret = ret;
/* Write ret before state. */
smp_wmb();
req->state = THREAD_DONE;
qemu_mutex_lock(>lock);
qemu_bh_schedule(pool->completion_bh);
The "pool->completion_bh" is function "thread_pool_completion_bh", which calls
"elem->common.cb(elem->common.opaque, elem->ret);". And the "elem->common.cb"
is function "bdrv_co_io_em_complete":
static void bdrv_co_io_em_complete(void *opaque, int ret)
{
CoroutineIOCompletion *co = opaque;
co->ret = ret;
qemu_coroutine_enter(co->coroutine, NULL);
}
5. Finally the return value(zero) will be stored in "co.ret" in function
"bdrv_co_io_em". However, what would happen if the req haven't done but the
function "bdrv_co_io_em" returned? The write operation would return an
uninitialized value(is it zero? I don't know.), is it?
Maybe these made the program report nothing with writer errors, I think.
Thanks again in advance.
Guangmu Zhu
-
Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
>
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
> which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
> "BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
> option to change the default behavior of the error report.
>
> So I think if there were some ways to change the default value of the error
>
Re: [Qemu-devel] No error report when using the qemu-img.exe toconvert a disk to vmdk format which is saved on a disk that has no morespace
If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
called instead. In function "vmdk_write_extent" I see "ret =
bdrv_pwrite(extent->file, write_offset, write_buf, write_len);". So the
"extend->file" is "bdrv_file", is it?
Thanks.
Guangmu Zhu
-
Correct a mistake:
So though the "count" would be "-EINVAL" if error occurred while writing some
file, the return value will always be zero. Maybe I missed something?
Sorry.
Guangmu Zhu
-
Thanks for your reply.
I read the source code again and have some question:
1. qume-img wrote the target file in the function "bdrv_aligned_pwritev" which
called the "drv->bdrv_co_writev" function. I haven't know how the qume-img
found the right driver for "drv", but I guessed it's "BlockDriver bdrv_file"
for the file on Windows, is it?
2. If the above is correct, the member "bdrv_aio_writev" of "BlockDriver" is
the function "raw_aio_writev" while the member "bdrv_co_writev" is NULL. The
function "bdrv_register" registers the "BlockDriver" and checks whether the
"bdrv_co_readv" or "bdrv_aio_readv" is NULL. If the "bdrv_co_readv" is NULL,
the member "bdrv_co_writev" is "bdrv_co_writev_em" which calls the function
"bdrv_co_io_em". There are some codes of it:
static int coroutine_fn bdrv_co_io_em(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, QEMUIOVector *iov,
bool is_write)
{
CoroutineIOCompletion co = {
.coroutine = qemu_coroutine_self(),
};
BlockAIOCB *acb;
if (is_write) {
acb = bs->drv->bdrv_aio_writev(bs, sector_num, iov, nb_sectors,
bdrv_co_io_em_complete, );
} else {
acb = bs->drv->bdrv_aio_readv(bs, sector_num, iov, nb_sectors,
bdrv_co_io_em_complete, );
}
trace_bdrv_co_io_em(bs, sector_num, nb_sectors, is_write, acb);
if (!acb) {
return -EIO;
}
qemu_coroutine_yield();
return co.ret;
}
3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
"raw-win32.c" and the quemu-img uses synchronous IO always, so the function
"paio_submit" in the same file will be called. This function submits the "aio"
to "worker_thread" with the callback "aio_worker". There are some codes in
"aio_worker":
ssize_t ret = 0;
..
case QEMU_AIO_WRITE:
count = handle_aiocb_rw(aiocb);
if (count == aiocb->aio_nbytes) {
count = 0;
} else {
count = -EINVAL;
}
break;
..
return ret;
So though the "count" would be zero if error occurred while writing some file,
the return value will always be zero. Maybe I missed something?
4. The function "worker_thread" calls the callback:
ret = req->func(req->arg);
req->ret = ret;
/* Write ret before state. */
smp_wmb();
req->state = THREAD_DONE;
qemu_mutex_lock(>lock);
qemu_bh_schedule(pool->completion_bh);
The "pool->completion_bh" is function "thread_pool_completion_bh", which calls
"elem->common.cb(elem->common.opaque, elem->ret);". And the "elem->common.cb"
is function "bdrv_co_io_em_complete":
static void bdrv_co_io_em_complete(void *opaque, int ret)
{
CoroutineIOCompletion *co = opaque;
co->ret = ret;
qemu_coroutine_enter(co->coroutine, NULL);
}
5. Finally the return value(zero) will be stored in "co.ret" in function
"bdrv_co_io_em". However, what would happen if the req haven't done but the
function "bdrv_co_io_em" returned? The write operation would return an
uninitialized value(is it zero? I don't know.), is it?
Maybe these made the program report nothing with writer errors, I think.
Thanks again in advance.
Guangmu Zhu
-
Am 22.09.2015 um 08:09 hat Guangmu Zhu geschrieben:
> I used the qemu-img.exe to convert a disk to vmdk format and the output file
> size could be 300 MB. However the left space of the disk the output file
> located on was about 200 MB. After a while, the left space had been zero but
> the program didn't stop or report any error. It was just going on as normal.
>
> I read the source code and found the error report was controlled by
> "BlockdevOnError on_read_error, on_write_erro
Re: [Qemu-devel] No error report when using the qemu-img.exetoconvert a disk to vmdk format which is saved on a disk that has nomorespace
I'll try the patch and report in a week for I'm too busy these days. And if I
could, I would like to help to maintain the Windows backend.
Sincerely.
Guangmu Zhu
-
Am 23.09.2015 um 13:30 hat Guangmu Zhu geschrieben:
> If the "BlockDriver" is "bdrv_vmdk", the function "vmdk_co_write" will be
> called instead. In function "vmdk_write_extent" I see "ret = bdrv_pwrite
> (extent->file, write_offset, write_buf, write_len);". So the "extend->file" is
> "bdrv_file", is it?
Yes, exactly. You'll go through bdrv_vmdk first, and then the nested
call goes to bdrv_file.
> -
>
> Correct a mistake:
> So though the "count" would be "-EINVAL" if error occurred while writing some
> file, the return value will always be zero. Maybe I missed something?
I think you're right. Instead of setting count = 0/-EINVAL in
aio_worker, we should be setting ret.
Can you try the patch below and report back?
> 3. The "bs->drv->bdrv_aio_writev" is function "raw_aio_writev" in file
> "raw-win32.c" and the quemu-img uses synchronous IO always, so the function
> "paio_submit" in the same file will be called. This function submits the "aio"
> to "worker_thread" with the callback "aio_worker". There are some codes in
> "aio_worker":
>
> ssize_t ret = 0;
> ..
> case QEMU_AIO_WRITE:
> count = handle_aiocb_rw(aiocb);
> if (count == aiocb->aio_nbytes) {
> count = 0;
> } else {
> count = -EINVAL;
> }
> break;
> ..
> return ret;
Independently of your problem, the code in aio_worker() looks a bit
fishy, because handle_aiocb_rw() can't distinguish between an error
and 0 bytes transferred.
For writes, that probably doesn't matter, but for reads, I think we
return a successful read of zeroes instead of signalling an error. This
might need another patch.
Generally, the Windows backend is not getting a lot of attention and
could use someone who checks it, cleans it up and fixes bugs.
Kevin
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 68f2338..b562c94 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -119,9 +119,9 @@ static int aio_worker(void *arg)
case QEMU_AIO_WRITE:
count = handle_aiocb_rw(aiocb);
if (count == aiocb->aio_nbytes) {
-count = 0;
+ret = 0;
} else {
-count = -EINVAL;
+ret = -EINVAL;
}
break;
case QEMU_AIO_FLUSH:
[Qemu-devel] No error report when using the qemu-img.exe to convert a disk to vmdk format which is saved on a disk that has no more space
Hi,
I used the qemu-img.exe to convert a disk to vmdk format and the output file
size could be 300 MB. However the left space of the disk the output file
located on was about 200 MB. After a while, the left space had been zero but
the program didn't stop or report any error. It was just going on as normal.
I read the source code and found the error report was controlled by
"BlockdevOnError on_read_error, on_write_error" in "struct BlockDriverState",
which had the default value "BLOCKDEV_ON_ERROR_REPORT" for "on_read_error" and
"BLOCKDEV_ON_ERROR_ENOSPC" for "on_writer_error". The qemu-img.exe had no
option to change the default behavior of the error report.
So I think if there were some ways to change the default value of the error
report, it might be better. Further more, I suggest we could just add some
codes to the "img_convert" function:
1827:out_blk = img_open("target", out_filename, out_fmt, flags, true,
quiet);
1828:if (!out_blk) {
1829:ret = -1;
1830:goto out;
1831:}
1832:out_bs = blk_bs(out_blk);
++ 1833:
++ 1834:bdrv_set_on_error(out_bs, BLOCKDEV_ON_ERROR_REPORT,
BLOCKDEV_ON_ERROR_REPORT);
Thanks,
Guangmu Zhu
