Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/ipmi/ipmi_bmc_extern.c | 4 +---
hw/ipmi/ipmi_bmc_sim.c | 7 ++-----
hw/ipmi/ipmi_bt.c | 7 +++----
hw/ipmi/ipmi_kcs.c | 3 +--
4 files changed, 7 insertions(+), 14 deletions(-)
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index 73b249fb60..ab9b66274d 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -213,7 +213,7 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b,
rsp[2] = err;
ibe->waiting_rsp = false;
k->handle_rsp(s, msg_id, rsp, 3);
- goto out;
+ return;
}
addchar(ibe, msg_id);
@@ -228,8 +228,6 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b,
/* Start the transmit */
continue_send(ibe);
-
- out:
}
static void handle_hw_op(IPMIBmcExtern *ibe, unsigned char hw_op)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index faec6fefb3..f4336946ce 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -463,13 +463,12 @@ void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log)
}
if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) {
- goto out;
+ return;
}
memcpy(ibs->evtbuf, evt, 16);
ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL;
k->set_atn(s, 1, attn_irq_enabled(ibs));
- out:
}
static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
uint8_t evd1, uint8_t evd2, uint8_t evd3)
@@ -979,7 +978,7 @@ static void get_msg(IPMIBmcSim *ibs,
if (QTAILQ_EMPTY(&ibs->rcvbufs)) {
rsp_buffer_set_error(rsp, 0x80); /* Queue empty */
- goto out;
+ return;
}
rsp_buffer_push(rsp, 0); /* Channel 0 */
msg = QTAILQ_FIRST(&ibs->rcvbufs);
@@ -994,8 +993,6 @@ static void get_msg(IPMIBmcSim *ibs,
ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE;
k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
}
-
-out:
}
static unsigned char
diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c
index 3ef1f435e7..f769cfa243 100644
--- a/hw/ipmi/ipmi_bt.c
+++ b/hw/ipmi/ipmi_bt.c
@@ -98,14 +98,14 @@ static void ipmi_bt_handle_event(IPMIInterface *ii)
IPMIBT *ib = iic->get_backend_data(ii);
if (ib->inlen < 4) {
- goto out;
+ return;
}
/* Note that overruns are handled by handle_command */
if (ib->inmsg[0] != (ib->inlen - 1)) {
/* Length mismatch, just ignore. */
IPMI_BT_SET_BBUSY(ib->control_reg, 1);
ib->inlen = 0;
- goto out;
+ return;
}
if ((ib->inmsg[1] == (IPMI_NETFN_APP << 2)) &&
(ib->inmsg[3] == IPMI_CMD_GET_BT_INTF_CAP)) {
@@ -136,7 +136,7 @@ static void ipmi_bt_handle_event(IPMIInterface *ii)
IPMI_BT_SET_B2H_IRQ(ib->mask_reg, 1);
ipmi_bt_raise_irq(ib);
}
- goto out;
+ return;
}
ib->waiting_seq = ib->inmsg[2];
ib->inmsg[2] = ib->inmsg[1];
@@ -145,7 +145,6 @@ static void ipmi_bt_handle_event(IPMIInterface *ii)
bk->handle_command(ib->bmc, ib->inmsg + 2, ib->inlen - 2,
sizeof(ib->inmsg), ib->waiting_rsp);
}
- out:
}
static void ipmi_bt_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
index f4f1523d6b..5bfc34676f 100644
--- a/hw/ipmi/ipmi_kcs.c
+++ b/hw/ipmi/ipmi_kcs.c
@@ -168,7 +168,7 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii)
ik->outpos = 0;
bk->handle_command(ik->bmc, ik->inmsg, ik->inlen, sizeof(ik->inmsg),
ik->waiting_rsp);
- goto out_noibf;
+ return;
} else if (ik->cmd_reg == IPMI_KCS_WRITE_END_CMD) {
ik->cmd_reg = -1;
ik->write_end = 1;
@@ -197,7 +197,6 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii)
ik->cmd_reg = -1;
ik->data_in_reg = -1;
IPMI_KCS_SET_IBF(ik->status_reg, 0);
- out_noibf:
}
static void ipmi_kcs_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
--
2.48.1Is this official coding style? I'm not a big fan of having return
statements in the middle of functions, I generally only put them at
the beginning or the end.
-corey
On Wed, Mar 19, 2025 at 10:26 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/ipmi/ipmi_bmc_extern.c | 4 +---
> hw/ipmi/ipmi_bmc_sim.c | 7 ++-----
> hw/ipmi/ipmi_bt.c | 7 +++----
> hw/ipmi/ipmi_kcs.c | 3 +--
> 4 files changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index 73b249fb60..ab9b66274d 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -213,7 +213,7 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b,
> rsp[2] = err;
> ibe->waiting_rsp = false;
> k->handle_rsp(s, msg_id, rsp, 3);
> - goto out;
> + return;
> }
>
> addchar(ibe, msg_id);
> @@ -228,8 +228,6 @@ static void ipmi_bmc_extern_handle_command(IPMIBmc *b,
>
> /* Start the transmit */
> continue_send(ibe);
> -
> - out:
> }
>
> static void handle_hw_op(IPMIBmcExtern *ibe, unsigned char hw_op)
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index faec6fefb3..f4336946ce 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -463,13 +463,12 @@ void ipmi_bmc_gen_event(IPMIBmc *b, uint8_t *evt, bool log)
> }
>
> if (ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL) {
> - goto out;
> + return;
> }
>
> memcpy(ibs->evtbuf, evt, 16);
> ibs->msg_flags |= IPMI_BMC_MSG_FLAG_EVT_BUF_FULL;
> k->set_atn(s, 1, attn_irq_enabled(ibs));
> - out:
> }
> static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
> uint8_t evd1, uint8_t evd2, uint8_t evd3)
> @@ -979,7 +978,7 @@ static void get_msg(IPMIBmcSim *ibs,
>
> if (QTAILQ_EMPTY(&ibs->rcvbufs)) {
> rsp_buffer_set_error(rsp, 0x80); /* Queue empty */
> - goto out;
> + return;
> }
> rsp_buffer_push(rsp, 0); /* Channel 0 */
> msg = QTAILQ_FIRST(&ibs->rcvbufs);
> @@ -994,8 +993,6 @@ static void get_msg(IPMIBmcSim *ibs,
> ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE;
> k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
> }
> -
> -out:
> }
>
> static unsigned char
> diff --git a/hw/ipmi/ipmi_bt.c b/hw/ipmi/ipmi_bt.c
> index 3ef1f435e7..f769cfa243 100644
> --- a/hw/ipmi/ipmi_bt.c
> +++ b/hw/ipmi/ipmi_bt.c
> @@ -98,14 +98,14 @@ static void ipmi_bt_handle_event(IPMIInterface *ii)
> IPMIBT *ib = iic->get_backend_data(ii);
>
> if (ib->inlen < 4) {
> - goto out;
> + return;
> }
> /* Note that overruns are handled by handle_command */
> if (ib->inmsg[0] != (ib->inlen - 1)) {
> /* Length mismatch, just ignore. */
> IPMI_BT_SET_BBUSY(ib->control_reg, 1);
> ib->inlen = 0;
> - goto out;
> + return;
> }
> if ((ib->inmsg[1] == (IPMI_NETFN_APP << 2)) &&
> (ib->inmsg[3] == IPMI_CMD_GET_BT_INTF_CAP)) {
> @@ -136,7 +136,7 @@ static void ipmi_bt_handle_event(IPMIInterface *ii)
> IPMI_BT_SET_B2H_IRQ(ib->mask_reg, 1);
> ipmi_bt_raise_irq(ib);
> }
> - goto out;
> + return;
> }
> ib->waiting_seq = ib->inmsg[2];
> ib->inmsg[2] = ib->inmsg[1];
> @@ -145,7 +145,6 @@ static void ipmi_bt_handle_event(IPMIInterface *ii)
> bk->handle_command(ib->bmc, ib->inmsg + 2, ib->inlen - 2,
> sizeof(ib->inmsg), ib->waiting_rsp);
> }
> - out:
> }
>
> static void ipmi_bt_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
> diff --git a/hw/ipmi/ipmi_kcs.c b/hw/ipmi/ipmi_kcs.c
> index f4f1523d6b..5bfc34676f 100644
> --- a/hw/ipmi/ipmi_kcs.c
> +++ b/hw/ipmi/ipmi_kcs.c
> @@ -168,7 +168,7 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii)
> ik->outpos = 0;
> bk->handle_command(ik->bmc, ik->inmsg, ik->inlen, sizeof(ik->inmsg),
> ik->waiting_rsp);
> - goto out_noibf;
> + return;
> } else if (ik->cmd_reg == IPMI_KCS_WRITE_END_CMD) {
> ik->cmd_reg = -1;
> ik->write_end = 1;
> @@ -197,7 +197,6 @@ static void ipmi_kcs_handle_event(IPMIInterface *ii)
> ik->cmd_reg = -1;
> ik->data_in_reg = -1;
> IPMI_KCS_SET_IBF(ik->status_reg, 0);
> - out_noibf:
> }
>
> static void ipmi_kcs_handle_rsp(IPMIInterface *ii, uint8_t msg_id,
> --
> 2.48.1
>
>
Corey Minyard <corey@minyard.net> writes: > Is this official coding style? I'm not a big fan of having return > statements in the middle of functions, I generally only put them at > the beginning or the end. There's nothing in docs/devel/style.rst. I count more than 42,000 return statements with indentation > 4. These are either within some block, or incorrectly indented. I'd bet my own money that it's the former for pretty much all of them. I count less than 130 labels right before a return statement at end of a function. Based on that, I'd say return in the middle of function is overwhelmingly common in our code.
Markus Armbruster <armbru@redhat.com> writes: > Corey Minyard <corey@minyard.net> writes: > >> Is this official coding style? I'm not a big fan of having return >> statements in the middle of functions, I generally only put them at >> the beginning or the end. > > There's nothing in docs/devel/style.rst. > > I count more than 42,000 return statements with indentation > 4. These > are either within some block, or incorrectly indented. I'd bet my own > money that it's the former for pretty much all of them. > > I count less than 130 labels right before a return statement at end of a > function. > > Based on that, I'd say return in the middle of function is > overwhelmingly common in our code. And with autofree variables it saves on clumsy goto and cleanup handlers. -- Alex Bennée Virtualisation Tech Lead @ Linaro
On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > Corey Minyard <corey@minyard.net> writes: > > > Is this official coding style? I'm not a big fan of having return > > statements in the middle of functions, I generally only put them at > > the beginning or the end. > > There's nothing in docs/devel/style.rst. > > I count more than 42,000 return statements with indentation > 4. These > are either within some block, or incorrectly indented. I'd bet my own > money that it's the former for pretty much all of them. > > I count less than 130 labels right before a return statement at end of a > function. > > Based on that, I'd say return in the middle of function is > overwhelmingly common in our code. > Ok. It's not a huge deal to me. I think it's more dangerous to have returns in the middle; they are easy to miss and an "out:" at the end make it more clear there are returns in the middle. But that's just my opinion. To make wholesale changes like this I would prefer it be in the style guide. But, I don't want to start a holy war, either. Sigh. I mean, just a "return;" at the end of a function, yes, that's a no-brainer, get rid of it. But that's not what the ones in the IPMI device are. Thanks, -corey
On Wed, Mar 19, 2025 at 02:42:21PM -0500, Corey Minyard wrote: > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > > Corey Minyard <corey@minyard.net> writes: > > > > > Is this official coding style? I'm not a big fan of having return > > > statements in the middle of functions, I generally only put them at > > > the beginning or the end. > > > > There's nothing in docs/devel/style.rst. > > > > I count more than 42,000 return statements with indentation > 4. These > > are either within some block, or incorrectly indented. I'd bet my own > > money that it's the former for pretty much all of them. > > > > I count less than 130 labels right before a return statement at end of a > > function. > > > > Based on that, I'd say return in the middle of function is > > overwhelmingly common in our code. > > > > Ok. It's not a huge deal to me. I think it's more dangerous to > have returns in the middle; they are easy to miss and an "out:" at the > end make it more clear there are returns in the middle. But that's > just my opinion. To make wholesale changes like this I would prefer > it be in the style guide. But, I don't want to start a holy war, > either. Sigh. In traditional C, I would agree with you that mid-function 'return's are often a bad idea, because they complicate free'ing of memory and tend to actively encourage memory/resource leaks. With our adoption of g_auto/g_autofree, that problem has been eliminated across a decently large subset of code. This swings the balance so that having mid-function 'return's often (but not always) results in shorter & easier to understand code, with few leak possiblities, provided g_auto/autofree is sufficient to deal with all cleanup needs. There will still be cases where it makes more sense to use 'goto' for cleanup, since g_auto/autofree is sufficient in all scenarios. Thus I don't think we should have a rule that strictly dictates either way. Better to leave it upto author's judgement call as to which approach results in clearer code for each particular function. I would still encourage maximising use of 'g_auto/autofree' where practical. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Corey Minyard <corey@minyard.net> writes: > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: >> Corey Minyard <corey@minyard.net> writes: >> >> > Is this official coding style? I'm not a big fan of having return >> > statements in the middle of functions, I generally only put them at >> > the beginning or the end. >> >> There's nothing in docs/devel/style.rst. >> >> I count more than 42,000 return statements with indentation > 4. These >> are either within some block, or incorrectly indented. I'd bet my own >> money that it's the former for pretty much all of them. >> >> I count less than 130 labels right before a return statement at end of a >> function. >> >> Based on that, I'd say return in the middle of function is >> overwhelmingly common in our code. >> > > Ok. It's not a huge deal to me. I think it's more dangerous to > have returns in the middle; they are easy to miss and an "out:" at the > end make it more clear there are returns in the middle. But that's > just my opinion. To make wholesale changes like this I would prefer > it be in the style guide. But, I don't want to start a holy war, > either. Sigh. > > I mean, just a "return;" at the end of a function, yes, that's a > no-brainer, get rid of it. But that's not what the ones in the IPMI > device are. Well, you're the maintainer there. If you'd like me to drop the five cases where return is directly after a label (all in hw/ipmi), I can do that for the low, low price of a "yes, please!"
On Wed, Mar 19, 2025 at 08:49:01PM +0100, Markus Armbruster wrote: > Corey Minyard <corey@minyard.net> writes: > > > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > >> Corey Minyard <corey@minyard.net> writes: > >> > >> > Is this official coding style? I'm not a big fan of having return > >> > statements in the middle of functions, I generally only put them at > >> > the beginning or the end. > >> > >> There's nothing in docs/devel/style.rst. > >> > >> I count more than 42,000 return statements with indentation > 4. These > >> are either within some block, or incorrectly indented. I'd bet my own > >> money that it's the former for pretty much all of them. > >> > >> I count less than 130 labels right before a return statement at end of a > >> function. > >> > >> Based on that, I'd say return in the middle of function is > >> overwhelmingly common in our code. > >> > > > > Ok. It's not a huge deal to me. I think it's more dangerous to > > have returns in the middle; they are easy to miss and an "out:" at the > > end make it more clear there are returns in the middle. But that's > > just my opinion. To make wholesale changes like this I would prefer > > it be in the style guide. But, I don't want to start a holy war, > > either. Sigh. > > > > I mean, just a "return;" at the end of a function, yes, that's a > > no-brainer, get rid of it. But that's not what the ones in the IPMI > > device are. > > Well, you're the maintainer there. If you'd like me to drop the five > cases where return is directly after a label (all in hw/ipmi), I can do > that for the low, low price of a "yes, please!" > No, I'm fine, I woudl just like it in the style guide. Signed-off-by: Corey Minyard <cminyard@mvista.com>
On Wed, Mar 19, 2025 at 03:51:45PM -0500, Corey Minyard wrote: > On Wed, Mar 19, 2025 at 08:49:01PM +0100, Markus Armbruster wrote: > > Corey Minyard <corey@minyard.net> writes: > > > > > On Wed, Mar 19, 2025 at 08:21:20PM +0100, Markus Armbruster wrote: > > >> Corey Minyard <corey@minyard.net> writes: > > >> > > >> > Is this official coding style? I'm not a big fan of having return > > >> > statements in the middle of functions, I generally only put them at > > >> > the beginning or the end. > > >> > > >> There's nothing in docs/devel/style.rst. > > >> > > >> I count more than 42,000 return statements with indentation > 4. These > > >> are either within some block, or incorrectly indented. I'd bet my own > > >> money that it's the former for pretty much all of them. > > >> > > >> I count less than 130 labels right before a return statement at end of a > > >> function. > > >> > > >> Based on that, I'd say return in the middle of function is > > >> overwhelmingly common in our code. > > >> > > > > > > Ok. It's not a huge deal to me. I think it's more dangerous to > > > have returns in the middle; they are easy to miss and an "out:" at the > > > end make it more clear there are returns in the middle. But that's > > > just my opinion. To make wholesale changes like this I would prefer > > > it be in the style guide. But, I don't want to start a holy war, > > > either. Sigh. > > > > > > I mean, just a "return;" at the end of a function, yes, that's a > > > no-brainer, get rid of it. But that's not what the ones in the IPMI > > > device are. > > > > Well, you're the maintainer there. If you'd like me to drop the five > > cases where return is directly after a label (all in hw/ipmi), I can do > > that for the low, low price of a "yes, please!" > > > > No, I'm fine, I woudl just like it in the style guide. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> I mean: Acked-by: Corey Minyard <cminyard@mvista.com>
© 2016 - 2025 Red Hat, Inc.