Re: [PATCH] vfio-ccw: Do not read region ret_code after write

[Cornelia Huck]

On Mon,  1 Mar 2021 20:51:43 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> A pwrite() call returns the number of bytes written (or -1 on error),
> and vfio-ccw compares this number with the size of the region to
> determine if an error had occurred or not. If they are equal, the
> code reads the ret_code field from the region. However, while the
> kernel sets the ret_code field as necessary, the region and thus
> this field is not "written back" to the user. So the value can only
> be what it was initialized to, which is zero.
> 
> Not harming anything, but it's a puzzle. Let's avoid the confusion
> and just set the return code to zero for this case.

Yes, ret_code seems to be pretty much useless for us: we don't even
look at it when we read the region for interrupt handling. Thankfully,
we don't seem to really need it, as we can rely on errno. (Probably
worth double checking that this is indeed the case.)

I don't suppose we need to handle a hypothetical broken kernel that
returns the wrong size with errno==0?

> 
> Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  hw/vfio/ccw.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index bc78a0ad76..bfd5fd07a5 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -106,7 +106,7 @@ again:
>          error_report("vfio-ccw: write I/O region failed with errno=%d", 
> errno);
>          ret = -errno;
>      } else {
> -        ret = region->ret_code;
> +        ret = 0;
>      }
>      switch (ret) {
>      case 0:
> @@ -194,7 +194,7 @@ again:
>          error_report("vfio-ccw: write cmd region failed with errno=%d", 
> errno);
>          ret = -errno;
>      } else {
> -        ret = region->ret_code;
> +        ret = 0;
>      }
>      switch (ret) {
>      case 0:
> @@ -234,7 +234,7 @@ again:
>          error_report("vfio-ccw: write cmd region failed with errno=%d", 
> errno);
>          ret = -errno;
>      } else {
> -        ret = region->ret_code;
> +        ret = 0;
>      }
>      switch (ret) {
>      case 0: