On Mon, 6 Jan 2020 15:24:03 -0300
Daniel Henrique Barboza <danielhb...@gmail.com> wrote:
> 'out' label from write_event_mask(), handle_sccb_read_events()
> and write_event_data() can be replaced by 'return'.
>
> CC: Cornelia Huck <coh...@redhat.com>
> CC: Halil Pasic <pa...@linux.ibm.com>
> CC: Christian Borntraeger <borntrae...@de.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
> ---
> hw/s390x/event-facility.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 6afe278cad..f3b9661f32 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -182,11 +182,11 @@ static void write_event_data(SCLPEventFacility *ef,
> SCCB *sccb)
> {
> if (sccb->h.function_code != SCLP_FC_NORMAL_WRITE) {
> sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> - goto out;
> + return;
> }
> if (be16_to_cpu(sccb->h.length) < 8) {
> sccb->h.response_code =
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> - goto out;
> + return;
> }
> /* first do a sanity check of the write events */
> sccb->h.response_code = cpu_to_be16(write_event_length_check(sccb));
> @@ -196,9 +196,6 @@ static void write_event_data(SCLPEventFacility *ef, SCCB
> *sccb)
> sccb->h.response_code =
> cpu_to_be16(handle_sccb_write_events(ef, sccb));
> }
> -
> -out:
> - return;
> }
I'm fine with the changes to write_event_data().
>
> static uint16_t handle_sccb_read_events(SCLPEventFacility *ef, SCCB *sccb,
> @@ -262,7 +259,7 @@ static void read_event_data(SCLPEventFacility *ef, SCCB
> *sccb)
>
> if (be16_to_cpu(sccb->h.length) != SCCB_SIZE) {
> sccb->h.response_code =
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> - goto out;
> + return;
> }
>
> sclp_cp_receive_mask = ef->receive_mask;
> @@ -280,18 +277,15 @@ static void read_event_data(SCLPEventFacility *ef, SCCB
> *sccb)
> (sclp_active_selection_mask & ~sclp_cp_receive_mask)) {
> sccb->h.response_code =
> cpu_to_be16(SCLP_RC_INVALID_SELECTION_MASK);
> - goto out;
> + return;
> }
> break;
> default:
> sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
> - goto out;
> + return;
> }
> sccb->h.response_code = cpu_to_be16(
> handle_sccb_read_events(ef, sccb, sclp_active_selection_mask));
> -
> -out:
> - return;
> }
I think read_event_data() is still a bit confusing, even if we get rid
of the 'out:' label, as the flow remains the same. How about something
like the following, which makes it clear that we handle the sccb events
for unconditional read and for a selective read with a valid mask:
diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 6afe278cad15..877721d6601e 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -262,17 +262,17 @@ static void read_event_data(SCLPEventFacility *ef, SCCB
*sccb)
if (be16_to_cpu(sccb->h.length) != SCCB_SIZE) {
sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
- goto out;
+ return;
}
- sclp_cp_receive_mask = ef->receive_mask;
-
- /* get active selection mask */
switch (sccb->h.function_code) {
case SCLP_UNCONDITIONAL_READ:
- sclp_active_selection_mask = sclp_cp_receive_mask;
+ sccb->h.response_code = cpu_to_be16(
+ handle_sccb_read_events(ef, sccb, ef->receive_mask));
break;
case SCLP_SELECTIVE_READ:
+ sclp_cp_receive_mask = ef->receive_mask;
+ /* get active selection mask */
copy_mask((uint8_t *)&sclp_active_selection_mask, (uint8_t
*)&red->mask,
sizeof(sclp_active_selection_mask), ef->mask_length);
sclp_active_selection_mask = be64_to_cpu(sclp_active_selection_mask);
@@ -280,18 +280,14 @@ static void read_event_data(SCLPEventFacility *ef, SCCB
*sccb)
(sclp_active_selection_mask & ~sclp_cp_receive_mask)) {
sccb->h.response_code =
cpu_to_be16(SCLP_RC_INVALID_SELECTION_MASK);
- goto out;
+ } else {
+ sccb->h.response_code = cpu_to_be16(
+ handle_sccb_read_events(ef, sccb, sclp_active_selection_mask));
}
break;
default:
sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_FUNCTION);
- goto out;
}
- sccb->h.response_code = cpu_to_be16(
- handle_sccb_read_events(ef, sccb, sclp_active_selection_mask));
-
-out:
- return;
}
static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
>
> static void write_event_mask(SCLPEventFacility *ef, SCCB *sccb)
> @@ -303,7 +297,7 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB
> *sccb)
> if (!mask_length || (mask_length > SCLP_EVENT_MASK_LEN_MAX) ||
> ((mask_length != 4) && !ef->allow_all_mask_sizes)) {
> sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_MASK_LENGTH);
> - goto out;
> + return;
> }
>
> /*
> @@ -328,9 +322,6 @@ static void write_event_mask(SCLPEventFacility *ef, SCCB
> *sccb)
>
> sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION);
> ef->mask_length = mask_length;
> -
> -out:
> - return;
> }
>
> /* qemu object creation and initialization functions */
The changes to write_event_mask() look fine to me as well.
