Re: [Qemu-devel] [PATCH] handle_aiocb_rw() can't distinguish between an error and 0 bytes transferred

[Kevin Wolf]

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 <address@hidden>

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