| Message ID | 160371605460.305923.5890143959901241157.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: > spapr_reallocate_hpt() has three users, two of which pass &error_fatal > and the third one, htab_load(), passes &local_err, uses it to detect > failures and simply propagates -EINVAL up to vmstate_load(), which will > cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt() > doesn't return right away when an error is detected in some cases. Also, > the comment suggesting that the caller is welcome to try to carry on > seems like a remnant in this respect. > > This can be improved: > - change spapr_reallocate_hpt() to always report a negative errno on > failure, either as reported by KVM or -ENOSPC if the HPT is smaller > than what was asked, > - use that to detect failures in htab_load() which is preferred over > checking &local_err, > - propagate this negative errno to vmstate_load() because it is more > accurate than propagating -EINVAL for all possible errors. > > Signed-off-by: Greg Kurz <groug@kaod.org> ... > -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > - Error **errp) > +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp) > { > ERRP_GUARD(); > long rc; > @@ -1496,7 +1495,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > if (rc == -EOPNOTSUPP) { > error_setg(errp, "HPT not supported in nested guests"); > - return; > + return -EOPNOTSUPP; > } > > if (rc < 0) { > @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > error_setg_errno(errp, errno, "Failed to allocate KVM HPT of order %d", > shift); > error_append_hint(errp, "Try smaller maxmem?\n"); > - /* This is almost certainly fatal, but if the caller really > - * wants to carry on with shift == 0, it's welcome to try */ > + return -errno; Maybe returning here should be in a previous patch. Otherwise patch looks good. > } else if (rc > 0) { > /* kernel-side HPT allocated */ > if (rc != shift) { > @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > "Requested order %d HPT, but kernel allocated order %ld", > shift, rc); > error_append_hint(errp, "Try smaller maxmem?\n"); > + return -ENOSPC; > } > > spapr->htab_shift = shift; > @@ -1533,6 +1532,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > /* We're setting up a hash table, so that means we're not radix */ > spapr->patb_entry = 0; > spapr_set_all_lpcrs(0, LPCR_HR | LPCR_UPRT); > + return 0; > } > > void spapr_setup_hpt(SpaprMachineState *spapr) > @@ -2286,11 +2286,13 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > } > > if (section_hdr) { > + int ret; > + > /* First section gives the htab size */ > - spapr_reallocate_hpt(spapr, section_hdr, &local_err); > - if (local_err) { > + ret = spapr_reallocate_hpt(spapr, section_hdr, &local_err); > + if (ret < 0) { > error_report_err(local_err); > - return -EINVAL; > + return ret; > } > return 0; > } ...
On Mon, 26 Oct 2020 14:49:34 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 10/26/20 1:40 PM, Greg Kurz wrote: > > spapr_reallocate_hpt() has three users, two of which pass &error_fatal > > and the third one, htab_load(), passes &local_err, uses it to detect > > failures and simply propagates -EINVAL up to vmstate_load(), which will > > cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt() > > doesn't return right away when an error is detected in some cases. Also, > > the comment suggesting that the caller is welcome to try to carry on > > seems like a remnant in this respect. > > > > This can be improved: > > - change spapr_reallocate_hpt() to always report a negative errno on > > failure, either as reported by KVM or -ENOSPC if the HPT is smaller > > than what was asked, > > - use that to detect failures in htab_load() which is preferred over > > checking &local_err, > > - propagate this negative errno to vmstate_load() because it is more > > accurate than propagating -EINVAL for all possible errors. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > ... > > > -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > - Error **errp) > > +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp) > > { > > ERRP_GUARD(); > > long rc; > > @@ -1496,7 +1495,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > > > if (rc == -EOPNOTSUPP) { > > error_setg(errp, "HPT not supported in nested guests"); > > - return; > > + return -EOPNOTSUPP; > > } > > > > if (rc < 0) { > > @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > error_setg_errno(errp, errno, "Failed to allocate KVM HPT of order %d", > > shift); > > error_append_hint(errp, "Try smaller maxmem?\n"); > > - /* This is almost certainly fatal, but if the caller really > > - * wants to carry on with shift == 0, it's welcome to try */ > > + return -errno; > > Maybe returning here should be in a previous patch. > Otherwise patch looks good. > It could have been indeed... > > } else if (rc > 0) { > > /* kernel-side HPT allocated */ > > if (rc != shift) { > > @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > "Requested order %d HPT, but kernel allocated order %ld", > > shift, rc); > > error_append_hint(errp, "Try smaller maxmem?\n"); > > + return -ENOSPC; ... along with this one. I didn't go this way because it doesn't really affect the final behavior since QEMU exits in all cases. It's mostly about propagating an appropriate errno up to VMState in the case of htab_load(). But if you find it clearer and I need to post a v2, I can certainly do that. > > } > > > > spapr->htab_shift = shift; > > @@ -1533,6 +1532,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > /* We're setting up a hash table, so that means we're not radix */ > > spapr->patb_entry = 0; > > spapr_set_all_lpcrs(0, LPCR_HR | LPCR_UPRT); > > + return 0; > > } > > > > void spapr_setup_hpt(SpaprMachineState *spapr) > > @@ -2286,11 +2286,13 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > > } > > > > if (section_hdr) { > > + int ret; > > + > > /* First section gives the htab size */ > > - spapr_reallocate_hpt(spapr, section_hdr, &local_err); > > - if (local_err) { > > + ret = spapr_reallocate_hpt(spapr, section_hdr, &local_err); > > + if (ret < 0) { > > error_report_err(local_err); > > - return -EINVAL; > > + return ret; > > } > > return 0; > > } > ... >
On Mon, Oct 26, 2020 at 01:40:54PM +0100, Greg Kurz wrote: > spapr_reallocate_hpt() has three users, two of which pass &error_fatal > and the third one, htab_load(), passes &local_err, uses it to detect > failures and simply propagates -EINVAL up to vmstate_load(), which will > cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt() > doesn't return right away when an error is detected in some cases. Also, > the comment suggesting that the caller is welcome to try to carry on > seems like a remnant in this respect. > > This can be improved: > - change spapr_reallocate_hpt() to always report a negative errno on > failure, either as reported by KVM or -ENOSPC if the HPT is smaller > than what was asked, > - use that to detect failures in htab_load() which is preferred over > checking &local_err, > - propagate this negative errno to vmstate_load() because it is more > accurate than propagating -EINVAL for all possible errors. > > Signed-off-by: Greg Kurz <groug@kaod.org> Applied to ppc-for-5.2, thanks. > --- > hw/ppc/spapr.c | 18 ++++++++++-------- > include/hw/ppc/spapr.h | 3 +-- > 2 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ff7de7da2875..12a012d9dd09 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1483,8 +1483,7 @@ void spapr_free_hpt(SpaprMachineState *spapr) > close_htab_fd(spapr); > } > > -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > - Error **errp) > +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp) > { > ERRP_GUARD(); > long rc; > @@ -1496,7 +1495,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > if (rc == -EOPNOTSUPP) { > error_setg(errp, "HPT not supported in nested guests"); > - return; > + return -EOPNOTSUPP; > } > > if (rc < 0) { > @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > error_setg_errno(errp, errno, "Failed to allocate KVM HPT of order %d", > shift); > error_append_hint(errp, "Try smaller maxmem?\n"); > - /* This is almost certainly fatal, but if the caller really > - * wants to carry on with shift == 0, it's welcome to try */ > + return -errno; > } else if (rc > 0) { > /* kernel-side HPT allocated */ > if (rc != shift) { > @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > "Requested order %d HPT, but kernel allocated order %ld", > shift, rc); > error_append_hint(errp, "Try smaller maxmem?\n"); > + return -ENOSPC; > } > > spapr->htab_shift = shift; > @@ -1533,6 +1532,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > /* We're setting up a hash table, so that means we're not radix */ > spapr->patb_entry = 0; > spapr_set_all_lpcrs(0, LPCR_HR | LPCR_UPRT); > + return 0; > } > > void spapr_setup_hpt(SpaprMachineState *spapr) > @@ -2286,11 +2286,13 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) > } > > if (section_hdr) { > + int ret; > + > /* First section gives the htab size */ > - spapr_reallocate_hpt(spapr, section_hdr, &local_err); > - if (local_err) { > + ret = spapr_reallocate_hpt(spapr, section_hdr, &local_err); > + if (ret < 0) { > error_report_err(local_err); > - return -EINVAL; > + return ret; > } > return 0; > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index bb47896f173b..2e89e36cfbdc 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -846,8 +846,7 @@ void spapr_hotplug_req_add_by_count_indexed(SpaprDrcType drc_type, > void spapr_hotplug_req_remove_by_count_indexed(SpaprDrcType drc_type, > uint32_t count, uint32_t index); > int spapr_hpt_shift_for_ramsize(uint64_t ramsize); > -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > - Error **errp); > +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp); > void spapr_clear_pending_events(SpaprMachineState *spapr); > void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr); > int spapr_max_server_number(SpaprMachineState *spapr); > >
On 10/26/20 3:47 PM, Greg Kurz wrote: > On Mon, 26 Oct 2020 14:49:34 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> On 10/26/20 1:40 PM, Greg Kurz wrote: >>> spapr_reallocate_hpt() has three users, two of which pass &error_fatal >>> and the third one, htab_load(), passes &local_err, uses it to detect >>> failures and simply propagates -EINVAL up to vmstate_load(), which will >>> cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt() >>> doesn't return right away when an error is detected in some cases. Also, >>> the comment suggesting that the caller is welcome to try to carry on >>> seems like a remnant in this respect. >>> >>> This can be improved: >>> - change spapr_reallocate_hpt() to always report a negative errno on >>> failure, either as reported by KVM or -ENOSPC if the HPT is smaller >>> than what was asked, >>> - use that to detect failures in htab_load() which is preferred over >>> checking &local_err, >>> - propagate this negative errno to vmstate_load() because it is more >>> accurate than propagating -EINVAL for all possible errors. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> ... >>> if (rc < 0) { >>> @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, >>> error_setg_errno(errp, errno, "Failed to allocate KVM HPT of order %d", >>> shift); >>> error_append_hint(errp, "Try smaller maxmem?\n"); >>> - /* This is almost certainly fatal, but if the caller really >>> - * wants to carry on with shift == 0, it's welcome to try */ >>> + return -errno; >> >> Maybe returning here should be in a previous patch. >> Otherwise patch looks good. >> > > It could have been indeed... > >>> } else if (rc > 0) { >>> /* kernel-side HPT allocated */ >>> if (rc != shift) { >>> @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, >>> "Requested order %d HPT, but kernel allocated order %ld", >>> shift, rc); >>> error_append_hint(errp, "Try smaller maxmem?\n"); >>> + return -ENOSPC; > > ... along with this one. > > I didn't go this way because it doesn't really affect the final behavior since > QEMU exits in all cases. It's mostly about propagating an appropriate errno up > to VMState in the case of htab_load(). But if you find it clearer and I need > to post a v2, I can certainly do that. As a reviewer I prefer dumb obvious patches I can quickly understand. If I stop, spend too long, am not sure, I spend time to ask, and usually stop reviewing the series. Then you spend time to answer, eventually respin. In doubt, KISS. Patch is queued so no need for v2.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index ff7de7da2875..12a012d9dd09 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1483,8 +1483,7 @@ void spapr_free_hpt(SpaprMachineState *spapr) close_htab_fd(spapr); } -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, - Error **errp) +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp) { ERRP_GUARD(); long rc; @@ -1496,7 +1495,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, if (rc == -EOPNOTSUPP) { error_setg(errp, "HPT not supported in nested guests"); - return; + return -EOPNOTSUPP; } if (rc < 0) { @@ -1504,8 +1503,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, error_setg_errno(errp, errno, "Failed to allocate KVM HPT of order %d", shift); error_append_hint(errp, "Try smaller maxmem?\n"); - /* This is almost certainly fatal, but if the caller really - * wants to carry on with shift == 0, it's welcome to try */ + return -errno; } else if (rc > 0) { /* kernel-side HPT allocated */ if (rc != shift) { @@ -1513,6 +1511,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, "Requested order %d HPT, but kernel allocated order %ld", shift, rc); error_append_hint(errp, "Try smaller maxmem?\n"); + return -ENOSPC; } spapr->htab_shift = shift; @@ -1533,6 +1532,7 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, /* We're setting up a hash table, so that means we're not radix */ spapr->patb_entry = 0; spapr_set_all_lpcrs(0, LPCR_HR | LPCR_UPRT); + return 0; } void spapr_setup_hpt(SpaprMachineState *spapr) @@ -2286,11 +2286,13 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id) } if (section_hdr) { + int ret; + /* First section gives the htab size */ - spapr_reallocate_hpt(spapr, section_hdr, &local_err); - if (local_err) { + ret = spapr_reallocate_hpt(spapr, section_hdr, &local_err); + if (ret < 0) { error_report_err(local_err); - return -EINVAL; + return ret; } return 0; } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index bb47896f173b..2e89e36cfbdc 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -846,8 +846,7 @@ void spapr_hotplug_req_add_by_count_indexed(SpaprDrcType drc_type, void spapr_hotplug_req_remove_by_count_indexed(SpaprDrcType drc_type, uint32_t count, uint32_t index); int spapr_hpt_shift_for_ramsize(uint64_t ramsize); -void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, - Error **errp); +int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp); void spapr_clear_pending_events(SpaprMachineState *spapr); void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr); int spapr_max_server_number(SpaprMachineState *spapr);
spapr_reallocate_hpt() has three users, two of which pass &error_fatal and the third one, htab_load(), passes &local_err, uses it to detect failures and simply propagates -EINVAL up to vmstate_load(), which will cause QEMU to exit. It is thus confusing that spapr_reallocate_hpt() doesn't return right away when an error is detected in some cases. Also, the comment suggesting that the caller is welcome to try to carry on seems like a remnant in this respect. This can be improved: - change spapr_reallocate_hpt() to always report a negative errno on failure, either as reported by KVM or -ENOSPC if the HPT is smaller than what was asked, - use that to detect failures in htab_load() which is preferred over checking &local_err, - propagate this negative errno to vmstate_load() because it is more accurate than propagating -EINVAL for all possible errors. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 18 ++++++++++-------- include/hw/ppc/spapr.h | 3 +-- 2 files changed, 11 insertions(+), 10 deletions(-)