| Message ID | 1449792685-17000-4-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 11, 2015 at 5:41 AM, David Gibson <david@gibson.dropbear.id.au> wrote: > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle > all errors with error_setg(&error_abort, ...). That's correct for > spapr_reset_htab() - if anything goes wrong there, there's really nothing > we can do about it. For spapr_alloc_htab() &error_fatal would make more > sense, since it occurs during initialization. > > But in any case the callers are really better placed to decide on the error > handling. So, instead make the functions use the error propagation > infrastructure. > > While we're at it improve the messages themselves a bit, and clean up the > indentation a little. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 91396cc..85474ee 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1015,7 +1015,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu) > #define CLEAN_HPTE(_hpte) ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY)) > #define DIRTY_HPTE(_hpte) ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY)) > > -static void spapr_alloc_htab(sPAPRMachineState *spapr) > +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp) > { > long shift; > int index; > @@ -1030,7 +1030,9 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) > * For HV KVM, host kernel will return -ENOMEM when requested > * HTAB size can't be allocated. > */ > - error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem"); > + error_setg(errp, > + "Error allocating KVM hash page table, try smaller maxmem: %s", > + strerror(-shift)); Do you want to explicitly "return" from here and other places in this patch like you do in 4/11 to improve readability ? Regards, Bharata.
On 12/10/2015 05:11 PM, David Gibson wrote: > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle > all errors with error_setg(&error_abort, ...). That's correct for > spapr_reset_htab() - if anything goes wrong there, there's really nothing > we can do about it. For spapr_alloc_htab() &error_fatal would make more > sense, since it occurs during initialization. > > But in any case the callers are really better placed to decide on the error > handling. So, instead make the functions use the error propagation > infrastructure. > > While we're at it improve the messages themselves a bit, and clean up the > indentation a little. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > @@ -1030,7 +1030,9 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) > * For HV KVM, host kernel will return -ENOMEM when requested > * HTAB size can't be allocated. > */ > - error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem"); > + error_setg(errp, > + "Error allocating KVM hash page table, try smaller maxmem: %s", > + strerror(-shift)); error_setg_errno() is nicer than calling strerror().
On Fri, Dec 11, 2015 at 02:10:40PM +0530, Bharata B Rao wrote: > On Fri, Dec 11, 2015 at 5:41 AM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle > > all errors with error_setg(&error_abort, ...). That's correct for > > spapr_reset_htab() - if anything goes wrong there, there's really nothing > > we can do about it. For spapr_alloc_htab() &error_fatal would make more > > sense, since it occurs during initialization. > > > > But in any case the callers are really better placed to decide on the error > > handling. So, instead make the functions use the error propagation > > infrastructure. > > > > While we're at it improve the messages themselves a bit, and clean up the > > indentation a little. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 26 ++++++++++++++++++-------- > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 91396cc..85474ee 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1015,7 +1015,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu) > > #define CLEAN_HPTE(_hpte) ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY)) > > #define DIRTY_HPTE(_hpte) ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY)) > > > > -static void spapr_alloc_htab(sPAPRMachineState *spapr) > > +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp) > > { > > long shift; > > int index; > > @@ -1030,7 +1030,9 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) > > * For HV KVM, host kernel will return -ENOMEM when requested > > * HTAB size can't be allocated. > > */ > > - error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem"); > > + error_setg(errp, > > + "Error allocating KVM hash page table, try smaller maxmem: %s", > > + strerror(-shift)); > > Do you want to explicitly "return" from here and other places in this > patch like you do in 4/11 to improve readability ? Given we still need the if (shift > 0), I don't think it really makes any difference to readability.
On Fri, Dec 11, 2015 at 08:01:39AM -0700, Eric Blake wrote: > On 12/10/2015 05:11 PM, David Gibson wrote: > > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle > > all errors with error_setg(&error_abort, ...). That's correct for > > spapr_reset_htab() - if anything goes wrong there, there's really nothing > > we can do about it. For spapr_alloc_htab() &error_fatal would make more > > sense, since it occurs during initialization. > > > > But in any case the callers are really better placed to decide on the error > > handling. So, instead make the functions use the error propagation > > infrastructure. > > > > While we're at it improve the messages themselves a bit, and clean up the > > indentation a little. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 26 ++++++++++++++++++-------- > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > @@ -1030,7 +1030,9 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) > > * For HV KVM, host kernel will return -ENOMEM when requested > > * HTAB size can't be allocated. > > */ > > - error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem"); > > + error_setg(errp, > > + "Error allocating KVM hash page table, try smaller maxmem: %s", > > + strerror(-shift)); > > error_setg_errno() is nicer than calling strerror(). Will adjust in next spin, thanks.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 91396cc..85474ee 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1015,7 +1015,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu) #define CLEAN_HPTE(_hpte) ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY)) #define DIRTY_HPTE(_hpte) ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY)) -static void spapr_alloc_htab(sPAPRMachineState *spapr) +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp) { long shift; int index; @@ -1030,7 +1030,9 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) * For HV KVM, host kernel will return -ENOMEM when requested * HTAB size can't be allocated. */ - error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem"); + error_setg(errp, + "Error allocating KVM hash page table, try smaller maxmem: %s", + strerror(-shift)); } else if (shift > 0) { /* * Kernel handles htab, we don't need to allocate one @@ -1039,7 +1041,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) * but we don't allow booting of such guests. */ if (shift != spapr->htab_shift) { - error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem"); + error_setg(errp, + "Small allocation for KVM hash page table (%ld < %" + PRIu32 "), try smaller maxmem", + shift, spapr->htab_shift); } spapr->htab_shift = shift; @@ -1063,17 +1068,22 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr) * If host kernel has allocated HTAB, KVM_PPC_ALLOCATE_HTAB ioctl is * used to clear HTAB. Otherwise QEMU-allocated HTAB is cleared manually. */ -static void spapr_reset_htab(sPAPRMachineState *spapr) +static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp) { long shift; int index; shift = kvmppc_reset_htab(spapr->htab_shift); if (shift < 0) { - error_setg(&error_abort, "Failed to reset HTAB"); + error_setg(errp, + "Error resetting KVM hash page table, try smaller maxmem: %s", + strerror(-shift)); } else if (shift > 0) { if (shift != spapr->htab_shift) { - error_setg(&error_abort, "Requested HTAB allocation failed during reset"); + error_setg(errp, + "Reduced size on reset of KVM hash page table (%ld < %" + PRIu32 "), try smaller maxmem", + shift, spapr->htab_shift); } /* Tell readers to update their file descriptor */ @@ -1144,7 +1154,7 @@ static void ppc_spapr_reset(void) foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); /* Reset the hash table & recalc the RMA */ - spapr_reset_htab(spapr); + spapr_reset_htab(spapr, &error_abort); qemu_devices_reset(); @@ -1784,7 +1794,7 @@ static void ppc_spapr_init(MachineState *machine) } spapr->htab_shift++; } - spapr_alloc_htab(spapr); + spapr_alloc_htab(spapr, &error_fatal); /* Set up Interrupt Controller before we create the VCPUs */ spapr->icp = xics_system_init(machine,
The spapr_alloc_htab() and spapr_reset_htab() functions currently handle all errors with error_setg(&error_abort, ...). That's correct for spapr_reset_htab() - if anything goes wrong there, there's really nothing we can do about it. For spapr_alloc_htab() &error_fatal would make more sense, since it occurs during initialization. But in any case the callers are really better placed to decide on the error handling. So, instead make the functions use the error propagation infrastructure. While we're at it improve the messages themselves a bit, and clean up the indentation a little. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)