diff mbox series

[3/4] target/ppc: Fix kvmppc_load_htab_chunk() error reporting

Message ID 160371604713.305923.5264900354159029580.stgit@bahia.lan
State Superseded
Headers show
Series spapr: Error handling fixes and cleanups (round 5) | expand

Commit Message

Greg Kurz Oct. 26, 2020, 12:40 p.m. UTC
If kvmppc_load_htab_chunk() fails, its return value is propagated up
to vmstate_load(). It should thus be a negative errno, not -1 (which
maps to EPERM and would lure the user into thinking that the problem
is necessarily related to a lack of privilege).

Return the error reported by KVM or ENOSPC in case of short write.
While here, propagate the error message through an @errp argument
and have the caller to print it with error_report_err() instead
of relying on fprintf().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c       |    4 +++-
 target/ppc/kvm.c     |   11 +++++------
 target/ppc/kvm_ppc.h |    5 +++--
 3 files changed, 11 insertions(+), 9 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 26, 2020, 1:45 p.m. UTC | #1
On 10/26/20 1:40 PM, Greg Kurz wrote:
> If kvmppc_load_htab_chunk() fails, its return value is propagated up

> to vmstate_load(). It should thus be a negative errno, not -1 (which

> maps to EPERM and would lure the user into thinking that the problem

> is necessarily related to a lack of privilege).

> 

> Return the error reported by KVM or ENOSPC in case of short write.

> While here, propagate the error message through an @errp argument

> and have the caller to print it with error_report_err() instead

> of relying on fprintf().

> 

> Signed-off-by: Greg Kurz <groug@kaod.org>

> ---

>   hw/ppc/spapr.c       |    4 +++-

>   target/ppc/kvm.c     |   11 +++++------

>   target/ppc/kvm_ppc.h |    5 +++--

>   3 files changed, 11 insertions(+), 9 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
David Gibson Oct. 27, 2020, 2 a.m. UTC | #2
On Mon, Oct 26, 2020 at 01:40:47PM +0100, Greg Kurz wrote:
> If kvmppc_load_htab_chunk() fails, its return value is propagated up

> to vmstate_load(). It should thus be a negative errno, not -1 (which

> maps to EPERM and would lure the user into thinking that the problem

> is necessarily related to a lack of privilege).

> 

> Return the error reported by KVM or ENOSPC in case of short write.

> While here, propagate the error message through an @errp argument

> and have the caller to print it with error_report_err() instead

> of relying on fprintf().

> 

> Signed-off-by: Greg Kurz <groug@kaod.org>

> ---

>  hw/ppc/spapr.c       |    4 +++-

>  target/ppc/kvm.c     |   11 +++++------

>  target/ppc/kvm_ppc.h |    5 +++--

>  3 files changed, 11 insertions(+), 9 deletions(-)

> 

> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c

> index f51b663f7dcb..ff7de7da2875 100644

> --- a/hw/ppc/spapr.c

> +++ b/hw/ppc/spapr.c

> @@ -2341,8 +2341,10 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)

>  

>              assert(fd >= 0);

>  

> -            rc = kvmppc_load_htab_chunk(f, fd, index, n_valid, n_invalid);

> +            rc = kvmppc_load_htab_chunk(f, fd, index, n_valid, n_invalid,

> +                                        &local_err);

>              if (rc < 0) {

> +                error_report_err(local_err);

>                  return rc;

>              }

>          }

> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c

> index d85ba8ffe00b..0223b93ea561 100644

> --- a/target/ppc/kvm.c

> +++ b/target/ppc/kvm.c

> @@ -2683,7 +2683,7 @@ int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)

>  }

>  

>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,

> -                           uint16_t n_valid, uint16_t n_invalid)

> +                           uint16_t n_valid, uint16_t n_invalid, Error **errp)

>  {

>      struct kvm_get_htab_header *buf;

>      size_t chunksize = sizeof(*buf) + n_valid * HASH_PTE_SIZE_64;

> @@ -2698,14 +2698,13 @@ int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,

>  

>      rc = write(fd, buf, chunksize);

>      if (rc < 0) {

> -        fprintf(stderr, "Error writing KVM hash table: %s\n",

> -                strerror(errno));

> -        return rc;

> +        error_setg_errno(errp, errno, "Error writing the KVM hash table");

> +        return -errno;

>      }

>      if (rc != chunksize) {

>          /* We should never get a short write on a single chunk */

> -        fprintf(stderr, "Short write, restoring KVM hash table\n");

> -        return -1;

> +        error_setg(errp, "Short write while restoring the KVM hash table");

> +        return -ENOSPC;


I'm not entirely sure -ENOSPC is the right choice here - this
indicates that the kernel interface is not behaving as we expect.  But
I can't immediately think of what's a better choice, so, applied to
ppc-for-5.2.


>      }

>      return 0;

>  }

> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h

> index 72e05f1cd2fc..73ce2bc95114 100644

> --- a/target/ppc/kvm_ppc.h

> +++ b/target/ppc/kvm_ppc.h

> @@ -56,7 +56,7 @@ int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);

>  int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp);

>  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);

>  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,

> -                           uint16_t n_valid, uint16_t n_invalid);

> +                           uint16_t n_valid, uint16_t n_invalid, Error **errp);

>  void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n);

>  void kvmppc_write_hpte(hwaddr ptex, uint64_t pte0, uint64_t pte1);

>  bool kvmppc_has_cap_fixup_hcalls(void);

> @@ -316,7 +316,8 @@ static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,

>  }

>  

>  static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,

> -                                         uint16_t n_valid, uint16_t n_invalid)

> +                                         uint16_t n_valid, uint16_t n_invalid,

> +                                         Error **errp)

>  {

>      abort();

>  }

> 

> 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f51b663f7dcb..ff7de7da2875 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2341,8 +2341,10 @@  static int htab_load(QEMUFile *f, void *opaque, int version_id)
 
             assert(fd >= 0);
 
-            rc = kvmppc_load_htab_chunk(f, fd, index, n_valid, n_invalid);
+            rc = kvmppc_load_htab_chunk(f, fd, index, n_valid, n_invalid,
+                                        &local_err);
             if (rc < 0) {
+                error_report_err(local_err);
                 return rc;
             }
         }
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index d85ba8ffe00b..0223b93ea561 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2683,7 +2683,7 @@  int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns)
 }
 
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
-                           uint16_t n_valid, uint16_t n_invalid)
+                           uint16_t n_valid, uint16_t n_invalid, Error **errp)
 {
     struct kvm_get_htab_header *buf;
     size_t chunksize = sizeof(*buf) + n_valid * HASH_PTE_SIZE_64;
@@ -2698,14 +2698,13 @@  int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
 
     rc = write(fd, buf, chunksize);
     if (rc < 0) {
-        fprintf(stderr, "Error writing KVM hash table: %s\n",
-                strerror(errno));
-        return rc;
+        error_setg_errno(errp, errno, "Error writing the KVM hash table");
+        return -errno;
     }
     if (rc != chunksize) {
         /* We should never get a short write on a single chunk */
-        fprintf(stderr, "Short write, restoring KVM hash table\n");
-        return -1;
+        error_setg(errp, "Short write while restoring the KVM hash table");
+        return -ENOSPC;
     }
     return 0;
 }
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 72e05f1cd2fc..73ce2bc95114 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -56,7 +56,7 @@  int kvmppc_define_rtas_kernel_token(uint32_t token, const char *function);
 int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
-                           uint16_t n_valid, uint16_t n_invalid);
+                           uint16_t n_valid, uint16_t n_invalid, Error **errp);
 void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n);
 void kvmppc_write_hpte(hwaddr ptex, uint64_t pte0, uint64_t pte1);
 bool kvmppc_has_cap_fixup_hcalls(void);
@@ -316,7 +316,8 @@  static inline int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize,
 }
 
 static inline int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
-                                         uint16_t n_valid, uint16_t n_invalid)
+                                         uint16_t n_valid, uint16_t n_invalid,
+                                         Error **errp)
 {
     abort();
 }