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;
}
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();
}
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>
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
© 2016 - 2025 Red Hat, Inc.