| Message ID | 160371604713.305923.5264900354159029580.stgit@bahia.lan |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 5) | expand |
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
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(); }
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(-)