| Message ID | tencent_0485DE1E4591564659722C75@qq.com |
|---|---|
| State | New |
| Headers | show |
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 <guangmuzhu@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, &ret_count, &ov); > } > 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, &err); > + 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, &err); > + if (err) { > + ret = -EINVAL; > } else { > - count = -EINVAL; > + if (count == aiocb->aio_nbytes) { > + count = 0; > + } else { > + count = -EINVAL; > + } > } This conflicts with the return value fix I sent. Can you base your patch on top of my fix? You can either apply my patch with 'git am', or you can pull the patches that are currently queued for master from git://repo.or.cz/qemu/kevin.git block > break; > case QEMU_AIO_FLUSH: > -- > 2.1.4 > > ------------------------------------- > > The handle_aiocb_rw() can't distinguish between an error and 0 bytes > transferred. > [...] Here you posted the whole patch a second time. I'm not 100% sure, but I would expect that this would confuse 'git am' as well. Do you want to send a v3 patch to try improving these points, or should I go forward with a version fixed up by myself (as an exception)? Kevin
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 <guangmuzhu@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, &ret_count, &ov); > } > 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, &err); > + 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, &err); > + if (err) { > + ret = -EINVAL; > } else { > - count = -EINVAL; > + if (count == aiocb->aio_nbytes) { > + count = 0; > + } else { > + count = -EINVAL; > + } > } This conflicts with the return value fix I sent. Can you base your patch on top of my fix? You can either apply my patch with 'git am', or you can pull the patches that are currently queued for master from git://repo.or.cz/qemu/kevin.git block > break; > case QEMU_AIO_FLUSH: > -- > 2.1.4 > > ------------------------------------- > > The handle_aiocb_rw() can't distinguish between an error and 0 bytes > transferred. > [...] Here you posted the whole patch a second time. I'm not 100% sure, but I would expect that this would confuse 'git am' as well. Do you want to send a v3 patch to try improving these points, or should I go forward with a version fixed up by myself (as an exception)? Kevin
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, &ret_count, &ov); } 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, &err); + 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, &err); + 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, &ret_count, &ov); } 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, &err); + 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, &err); + if (err) { + ret = -EINVAL; } else { - count = -EINVAL; + if (count == aiocb->aio_nbytes) { + count = 0; + } else { + count = -EINVAL; + } } break; case QEMU_AIO_FLUSH:
Correct patch format. Signed-off-by: Guangmu Zhu <guangmuzhu@gmail.com> --- block/raw-win32.c | 48 ++++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-)