Message ID | 1439967371-15870-3-git-send-email-bharata@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Quoting Bharata B Rao (2015-08-19 01:56:10) > drck->set_isolation_state() can return error. For such a case ensure > correct error is returned by rtas_set_indicator() instead of always > returning success. > > TODO: rtas_st(, , uint32 val) => the return value uint32, but > drck->set_[allocation/indicator/isolation]_state() is returning int. > Should we change this return value to uint32_t to match with rtas_st() > argument ? I wouldn't bother too much aligning the types unless we go to the extent of documenting these interfaces as returning rtas error codes. That's not really the case currently, and there's a lot of rtas errors that don't really need to be determined by DRC code so I think it's more trouble than it's worth. For now I think it's better to just check for ret != 0 and set return values explicitly in rtas code based on what the drc errors entail. > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > --- > hw/ppc/spapr_rtas.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index e99e25f..96729b4 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -374,6 +374,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t sensor_state; > sPAPRDRConnector *drc; > sPAPRDRConnectorClass *drck; > + int ret; > > if (nargs != 3 || nret != 1) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -413,19 +414,19 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, > spapr_ccs_remove(spapr, ccs); > } > } > - drck->set_isolation_state(drc, sensor_state); > + ret = drck->set_isolation_state(drc, sensor_state); > break; > case RTAS_SENSOR_TYPE_DR: > - drck->set_indicator_state(drc, sensor_state); > + ret = drck->set_indicator_state(drc, sensor_state); > break; > case RTAS_SENSOR_TYPE_ALLOCATION_STATE: > - drck->set_allocation_state(drc, sensor_state); > + ret = drck->set_allocation_state(drc, sensor_state); > break; > default: > goto out_unimplemented; > } > > - rtas_st(rets, 0, RTAS_OUT_SUCCESS); > + rtas_st(rets, 0, ret); > return; > > out_unimplemented: > -- > 2.1.0 >
On Wed, Aug 19, 2015 at 12:26:10PM +0530, Bharata B Rao wrote: > drck->set_isolation_state() can return error. For such a case ensure > correct error is returned by rtas_set_indicator() instead of always > returning success. > > TODO: rtas_st(, , uint32 val) => the return value uint32, but > drck->set_[allocation/indicator/isolation]_state() is returning int. > Should we change this return value to uint32_t to match with rtas_st() > argument ? > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_rtas.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index e99e25f..96729b4 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -374,6 +374,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, > uint32_t sensor_state; > sPAPRDRConnector *drc; > sPAPRDRConnectorClass *drck; > + int ret; > > if (nargs != 3 || nret != 1) { > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > @@ -413,19 +414,19 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, > spapr_ccs_remove(spapr, ccs); > } > } > - drck->set_isolation_state(drc, sensor_state); > + ret = drck->set_isolation_state(drc, sensor_state); > break; > case RTAS_SENSOR_TYPE_DR: > - drck->set_indicator_state(drc, sensor_state); > + ret = drck->set_indicator_state(drc, sensor_state); > break; > case RTAS_SENSOR_TYPE_ALLOCATION_STATE: > - drck->set_allocation_state(drc, sensor_state); > + ret = drck->set_allocation_state(drc, sensor_state); > break; > default: > goto out_unimplemented; > } > > - rtas_st(rets, 0, RTAS_OUT_SUCCESS); > + rtas_st(rets, 0, ret); > return; > > out_unimplemented:
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index e99e25f..96729b4 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -374,6 +374,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, uint32_t sensor_state; sPAPRDRConnector *drc; sPAPRDRConnectorClass *drck; + int ret; if (nargs != 3 || nret != 1) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -413,19 +414,19 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, spapr_ccs_remove(spapr, ccs); } } - drck->set_isolation_state(drc, sensor_state); + ret = drck->set_isolation_state(drc, sensor_state); break; case RTAS_SENSOR_TYPE_DR: - drck->set_indicator_state(drc, sensor_state); + ret = drck->set_indicator_state(drc, sensor_state); break; case RTAS_SENSOR_TYPE_ALLOCATION_STATE: - drck->set_allocation_state(drc, sensor_state); + ret = drck->set_allocation_state(drc, sensor_state); break; default: goto out_unimplemented; } - rtas_st(rets, 0, RTAS_OUT_SUCCESS); + rtas_st(rets, 0, ret); return; out_unimplemented:
drck->set_isolation_state() can return error. For such a case ensure correct error is returned by rtas_set_indicator() instead of always returning success. TODO: rtas_st(, , uint32 val) => the return value uint32, but drck->set_[allocation/indicator/isolation]_state() is returning int. Should we change this return value to uint32_t to match with rtas_st() argument ? Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/ppc/spapr_rtas.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)