qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement


From: Corey Minyard
Subject: Re: [Qemu-devel] [PATCH v2 2/9] ipmi: replace goto by a return statement
Date: Fri, 22 Jan 2016 06:56:51 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1

On 01/21/2016 11:18 AM, Cédric Le Goater wrote:
Each routine using the IPMI_ADD_RSP_DATA, IPMI_CHECK_CMD_LEN or
IPMI_CHECK_RESERVATION macros needs to define a goto label 'out' to
handle hidden errors. Using directly a return statement as the same
Using a return statement directly has the same....
effect and it removes the fact that 'out' needs to be defined.

The code exits in ipmi_sim_handle_command() are a little different
from the rest and a "possible" error in the macro IPMI_ADD_RSP_DATA is
handled before making use of it. This might be a bit excessive as a
minimum response len is currently 300 bytes and the patch checks that
at least 3 are available.

Yeah, it seems a little excessive.  The compiler should figure out that
the value is always false and remove the code.

The return in the macro seems to obfuscate the return as much as
the goto, but the code does look a lot neater this way.

Reviewed-by: Corey Minyard <address@hidden>


Signed-off-by: Cédric Le Goater <address@hidden>
---
  hw/ipmi/ipmi_bmc_sim.c | 140 +++++++++++++++----------------------------------
  1 file changed, 41 insertions(+), 99 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 0a59e539f549..e42c7e86c344 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -258,7 +258,7 @@ struct IPMIBmcSim {
      do {                                                   \
          if (*rsp_len >= max_rsp_len) {                     \
              rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;       \
-            goto out;                                      \
+            return;                                        \
          }                                                  \
          rsp[(*rsp_len)++] = (b);                           \
      } while (0)
@@ -267,7 +267,7 @@ struct IPMIBmcSim {
  #define IPMI_CHECK_CMD_LEN(l) \
      if (cmd_len < l) {                                     \
          rsp[2] = IPMI_CC_REQUEST_DATA_LENGTH_INVALID;      \
-        goto out; \
+        return; \
      }
/* Check that the reservation in the command is valid. */
@@ -275,7 +275,7 @@ struct IPMIBmcSim {
      do {                                                   \
          if ((cmd[off] | (cmd[off + 1] << 8)) != r) {       \
              rsp[2] = IPMI_CC_INVALID_RESERVATION;          \
-            goto out;                                      \
+            return;                                        \
          }                                                  \
      } while (0)
@@ -453,14 +453,12 @@ static void gen_event(IPMIBmcSim *ibs, unsigned int sens_num, uint8_t deassert,
      }
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:
-    return;
  }
static void sensor_set_discrete_bit(IPMIBmcSim *ibs, unsigned int sensor,
@@ -581,6 +579,11 @@ static void ipmi_sim_handle_command(IPMIBmc *b,
/* Set up the response, set the low bit of NETFN. */
      /* Note that max_rsp_len must be at least 3 */
+    if (max_rsp_len < 3) {
+        rsp[2] = IPMI_CC_REQUEST_DATA_TRUNCATED;
+        goto out;
+    }
+
      IPMI_ADD_RSP_DATA(cmd[0] | 0x04);
      IPMI_ADD_RSP_DATA(cmd[1]);
      IPMI_ADD_RSP_DATA(0); /* Assume success */
@@ -698,8 +701,6 @@ static void chassis_capabilities(IPMIBmcSim *ibs,
      IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
      IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
      IPMI_ADD_RSP_DATA(ibs->parent.slave_addr);
- out:
-    return;
  }
static void chassis_status(IPMIBmcSim *ibs,
@@ -711,8 +712,6 @@ static void chassis_status(IPMIBmcSim *ibs,
      IPMI_ADD_RSP_DATA(0);
      IPMI_ADD_RSP_DATA(0);
      IPMI_ADD_RSP_DATA(0);
- out:
-    return;
  }
static void chassis_control(IPMIBmcSim *ibs,
@@ -746,10 +745,8 @@ static void chassis_control(IPMIBmcSim *ibs,
          break;
      default:
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
- out:
-    return;
  }
static void get_device_id(IPMIBmcSim *ibs,
@@ -768,8 +765,6 @@ static void get_device_id(IPMIBmcSim *ibs,
      IPMI_ADD_RSP_DATA(ibs->mfg_id[2]);
      IPMI_ADD_RSP_DATA(ibs->product_id[0]);
      IPMI_ADD_RSP_DATA(ibs->product_id[1]);
- out:
-    return;
  }
static void set_global_enables(IPMIBmcSim *ibs, uint8_t val)
@@ -822,8 +817,6 @@ static void set_bmc_global_enables(IPMIBmcSim *ibs,
  {
      IPMI_CHECK_CMD_LEN(3);
      set_global_enables(ibs, cmd[2]);
- out:
-    return;
  }
static void get_bmc_global_enables(IPMIBmcSim *ibs,
@@ -832,8 +825,6 @@ static void get_bmc_global_enables(IPMIBmcSim *ibs,
                                     unsigned int max_rsp_len)
  {
      IPMI_ADD_RSP_DATA(ibs->bmc_global_enables);
- out:
-    return;
  }
static void clr_msg_flags(IPMIBmcSim *ibs,
@@ -847,8 +838,6 @@ static void clr_msg_flags(IPMIBmcSim *ibs,
      IPMI_CHECK_CMD_LEN(3);
      ibs->msg_flags &= ~cmd[2];
      k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
- out:
-    return;
  }
static void get_msg_flags(IPMIBmcSim *ibs,
@@ -857,8 +846,6 @@ static void get_msg_flags(IPMIBmcSim *ibs,
                            unsigned int max_rsp_len)
  {
      IPMI_ADD_RSP_DATA(ibs->msg_flags);
- out:
-    return;
  }
static void read_evt_msg_buf(IPMIBmcSim *ibs,
@@ -872,15 +859,13 @@ static void read_evt_msg_buf(IPMIBmcSim *ibs,
if (!(ibs->msg_flags & IPMI_BMC_MSG_FLAG_EVT_BUF_FULL)) {
          rsp[2] = 0x80;
-        goto out;
+        return;
      }
      for (i = 0; i < 16; i++) {
          IPMI_ADD_RSP_DATA(ibs->evtbuf[i]);
      }
      ibs->msg_flags &= ~IPMI_BMC_MSG_FLAG_EVT_BUF_FULL;
      k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
- out:
-    return;
  }
static void get_msg(IPMIBmcSim *ibs,
@@ -911,7 +896,7 @@ static void get_msg(IPMIBmcSim *ibs,
          k->set_atn(s, attn_set(ibs), attn_irq_enabled(ibs));
      }
- out:
+out:
      qemu_mutex_unlock(&ibs->lock);
      return;
  }
@@ -942,14 +927,14 @@ static void send_msg(IPMIBmcSim *ibs,
      if (cmd[2] != 0) {
          /* We only handle channel 0 with no options */
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
IPMI_CHECK_CMD_LEN(10);
      if (cmd[3] != 0x40) {
          /* We only emulate a MC at address 0x40. */
          rsp[2] = 0x83; /* NAK on write */
-        goto out;
+        return;
      }
cmd += 3; /* Skip the header. */
@@ -961,7 +946,7 @@ static void send_msg(IPMIBmcSim *ibs,
       */
      if (ipmb_checksum(cmd, cmd_len, 0) != 0 ||
          cmd[3] != 0x20) { /* Improper response address */
-        goto out; /* No response */
+        return; /* No response */
      }
netfn = cmd[1] >> 2;
@@ -971,7 +956,7 @@ static void send_msg(IPMIBmcSim *ibs,
if (rqLun != 2) {
          /* We only support LUN 2 coming back to us. */
-        goto out;
+        return;
      }
msg = g_malloc(sizeof(*msg));
@@ -1011,9 +996,6 @@ static void send_msg(IPMIBmcSim *ibs,
      ibs->msg_flags |= IPMI_BMC_MSG_FLAG_RCV_MSG_QUEUE;
      k->set_atn(s, 1, attn_irq_enabled(ibs));
      qemu_mutex_unlock(&ibs->lock);
-
- out:
-    return;
  }
static void do_watchdog_reset(IPMIBmcSim *ibs)
@@ -1042,11 +1024,9 @@ static void reset_watchdog_timer(IPMIBmcSim *ibs,
  {
      if (!ibs->watchdog_initialized) {
          rsp[2] = 0x80;
-        goto out;
+        return;
      }
      do_watchdog_reset(ibs);
- out:
-    return;
  }
static void set_watchdog_timer(IPMIBmcSim *ibs,
@@ -1062,7 +1042,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
      val = cmd[2] & 0x7; /* Validate use */
      if (val == 0 || val > 5) {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
      val = cmd[3] & 0x7; /* Validate action */
      switch (val) {
@@ -1086,7 +1066,7 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
      }
      if (rsp[2]) {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
val = (cmd[3] >> 4) & 0x7; /* Validate preaction */
@@ -1099,12 +1079,12 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
          if (!k->do_hw_op(s, IPMI_SEND_NMI, 1)) {
              /* NMI not supported. */
              rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-            goto out;
+            return;
          }
      default:
          /* We don't support PRE_SMI */
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
ibs->watchdog_initialized = 1;
@@ -1118,8 +1098,6 @@ static void set_watchdog_timer(IPMIBmcSim *ibs,
      } else {
          ibs->watchdog_running = 0;
      }
- out:
-    return;
  }
static void get_watchdog_timer(IPMIBmcSim *ibs,
@@ -1141,8 +1119,6 @@ static void get_watchdog_timer(IPMIBmcSim *ibs,
          IPMI_ADD_RSP_DATA(0);
          IPMI_ADD_RSP_DATA(0);
      }
- out:
-    return;
  }
static void get_sdr_rep_info(IPMIBmcSim *ibs,
@@ -1165,8 +1141,6 @@ static void get_sdr_rep_info(IPMIBmcSim *ibs,
      }
      /* Only modal support, reserve supported */
      IPMI_ADD_RSP_DATA((ibs->sdr.overflow << 7) | 0x22);
- out:
-    return;
  }
static void reserve_sdr_rep(IPMIBmcSim *ibs,
@@ -1176,8 +1150,6 @@ static void reserve_sdr_rep(IPMIBmcSim *ibs,
  {
      IPMI_ADD_RSP_DATA(ibs->sdr.reservation & 0xff);
      IPMI_ADD_RSP_DATA((ibs->sdr.reservation >> 8) & 0xff);
- out:
-    return;
  }
static void get_sdr(IPMIBmcSim *ibs,
@@ -1196,11 +1168,11 @@ static void get_sdr(IPMIBmcSim *ibs,
      if (sdr_find_entry(&ibs->sdr, cmd[4] | (cmd[5] << 8),
                         &pos, &nextrec)) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      if (cmd[6] > (ibs->sdr.sdr[pos + 4])) {
          rsp[2] = IPMI_CC_PARM_OUT_OF_RANGE;
-        goto out;
+        return;
      }
IPMI_ADD_RSP_DATA(nextrec & 0xff);
@@ -1212,12 +1184,10 @@ static void get_sdr(IPMIBmcSim *ibs,
if ((cmd[7] + *rsp_len) > max_rsp_len) {
          rsp[2] = IPMI_CC_CANNOT_RETURN_REQ_NUM_BYTES;
-        goto out;
+        return;
      }
      memcpy(rsp + *rsp_len, ibs->sdr.sdr + pos + cmd[6], cmd[7]);
      *rsp_len += cmd[7];
- out:
-    return;
  }
static void add_sdr(IPMIBmcSim *ibs,
@@ -1229,12 +1199,10 @@ static void add_sdr(IPMIBmcSim *ibs,
if (sdr_add_entry(ibs, cmd + 2, cmd_len - 2, &recid)) {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
      IPMI_ADD_RSP_DATA(recid & 0xff);
      IPMI_ADD_RSP_DATA((recid >> 8) & 0xff);
- out:
-    return;
  }
static void clear_sdr_rep(IPMIBmcSim *ibs,
@@ -1246,7 +1214,7 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
      IPMI_CHECK_RESERVATION(2, ibs->sdr.reservation);
      if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
      if (cmd[7] == 0xaa) {
          ibs->sdr.next_free = 0;
@@ -1258,10 +1226,8 @@ static void clear_sdr_rep(IPMIBmcSim *ibs,
          IPMI_ADD_RSP_DATA(1); /* Erasure complete */
      } else {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
- out:
-    return;
  }
static void get_sel_info(IPMIBmcSim *ibs,
@@ -1285,8 +1251,6 @@ static void get_sel_info(IPMIBmcSim *ibs,
      }
      /* Only support Reserve SEL */
      IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
- out:
-    return;
  }
static void reserve_sel(IPMIBmcSim *ibs,
@@ -1296,8 +1260,6 @@ static void reserve_sel(IPMIBmcSim *ibs,
  {
      IPMI_ADD_RSP_DATA(ibs->sel.reservation & 0xff);
      IPMI_ADD_RSP_DATA((ibs->sel.reservation >> 8) & 0xff);
- out:
-    return;
  }
static void get_sel_entry(IPMIBmcSim *ibs,
@@ -1313,17 +1275,17 @@ static void get_sel_entry(IPMIBmcSim *ibs,
      }
      if (ibs->sel.next_free == 0) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      if (cmd[6] > 15) {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
      if (cmd[7] == 0xff) {
          cmd[7] = 16;
      } else if ((cmd[7] + cmd[6]) > 16) {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      } else {
          cmd[7] += cmd[6];
      }
@@ -1333,7 +1295,7 @@ static void get_sel_entry(IPMIBmcSim *ibs,
          val = ibs->sel.next_free - 1;
      } else if (val >= ibs->sel.next_free) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      if ((val + 1) == ibs->sel.next_free) {
          IPMI_ADD_RSP_DATA(0xff);
@@ -1345,8 +1307,6 @@ static void get_sel_entry(IPMIBmcSim *ibs,
      for (; cmd[6] < cmd[7]; cmd[6]++) {
          IPMI_ADD_RSP_DATA(ibs->sel.sel[val][cmd[6]]);
      }
- out:
-    return;
  }
static void add_sel_entry(IPMIBmcSim *ibs,
@@ -1357,13 +1317,11 @@ static void add_sel_entry(IPMIBmcSim *ibs,
      IPMI_CHECK_CMD_LEN(18);
      if (sel_add_event(ibs, cmd + 2)) {
          rsp[2] = IPMI_CC_OUT_OF_SPACE;
-        goto out;
+        return;
      }
      /* sel_add_event fills in the record number. */
      IPMI_ADD_RSP_DATA(cmd[2]);
      IPMI_ADD_RSP_DATA(cmd[3]);
- out:
-    return;
  }
static void clear_sel(IPMIBmcSim *ibs,
@@ -1375,7 +1333,7 @@ static void clear_sel(IPMIBmcSim *ibs,
      IPMI_CHECK_RESERVATION(2, ibs->sel.reservation);
      if (cmd[4] != 'C' || cmd[5] != 'L' || cmd[6] != 'R') {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
      if (cmd[7] == 0xaa) {
          ibs->sel.next_free = 0;
@@ -1387,10 +1345,8 @@ static void clear_sel(IPMIBmcSim *ibs,
          IPMI_ADD_RSP_DATA(1); /* Erasure complete */
      } else {
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
- out:
-    return;
  }
static void get_sel_time(IPMIBmcSim *ibs,
@@ -1407,8 +1363,6 @@ static void get_sel_time(IPMIBmcSim *ibs,
      IPMI_ADD_RSP_DATA((val >> 8) & 0xff);
      IPMI_ADD_RSP_DATA((val >> 16) & 0xff);
      IPMI_ADD_RSP_DATA((val >> 24) & 0xff);
- out:
-    return;
  }
static void set_sel_time(IPMIBmcSim *ibs,
@@ -1423,8 +1377,6 @@ static void set_sel_time(IPMIBmcSim *ibs,
      val = cmd[2] | (cmd[3] << 8) | (cmd[4] << 16) | (cmd[5] << 24);
      ipmi_gettime(&now);
      ibs->sel.time_offset = now.tv_sec - ((long) val);
- out:
-    return;
  }
static void set_sensor_evt_enable(IPMIBmcSim *ibs,
@@ -1438,7 +1390,7 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
      if ((cmd[2] > MAX_SENSORS) ||
              !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      sens = ibs->sensors + cmd[2];
      switch ((cmd[3] >> 4) & 0x3) {
@@ -1474,11 +1426,9 @@ static void set_sensor_evt_enable(IPMIBmcSim *ibs,
          break;
      case 3:
          rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
-        goto out;
+        return;
      }
      IPMI_SENSOR_SET_RET_STATUS(sens, cmd[3]);
- out:
-    return;
  }
static void get_sensor_evt_enable(IPMIBmcSim *ibs,
@@ -1492,7 +1442,7 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
      if ((cmd[2] > MAX_SENSORS) ||
          !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      sens = ibs->sensors + cmd[2];
      IPMI_ADD_RSP_DATA(IPMI_SENSOR_GET_RET_STATUS(sens));
@@ -1500,8 +1450,6 @@ static void get_sensor_evt_enable(IPMIBmcSim *ibs,
      IPMI_ADD_RSP_DATA((sens->assert_enable >> 8) & 0xff);
      IPMI_ADD_RSP_DATA(sens->deassert_enable & 0xff);
      IPMI_ADD_RSP_DATA((sens->deassert_enable >> 8) & 0xff);
- out:
-    return;
  }
static void rearm_sensor_evts(IPMIBmcSim *ibs,
@@ -1515,17 +1463,15 @@ static void rearm_sensor_evts(IPMIBmcSim *ibs,
      if ((cmd[2] > MAX_SENSORS) ||
          !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      sens = ibs->sensors + cmd[2];
if ((cmd[3] & 0x80) == 0) {
          /* Just clear everything */
          sens->states = 0;
-        goto out;
+        return;
      }
- out:
-    return;
  }
static void get_sensor_evt_status(IPMIBmcSim *ibs,
@@ -1539,7 +1485,7 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
      if ((cmd[2] > MAX_SENSORS) ||
          !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      sens = ibs->sensors + cmd[2];
      IPMI_ADD_RSP_DATA(sens->reading);
@@ -1548,8 +1494,6 @@ static void get_sensor_evt_status(IPMIBmcSim *ibs,
      IPMI_ADD_RSP_DATA((sens->assert_states >> 8) & 0xff);
      IPMI_ADD_RSP_DATA(sens->deassert_states & 0xff);
      IPMI_ADD_RSP_DATA((sens->deassert_states >> 8) & 0xff);
- out:
-    return;
  }
static void get_sensor_reading(IPMIBmcSim *ibs,
@@ -1563,7 +1507,7 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
      if ((cmd[2] > MAX_SENSORS) ||
              !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
          rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
-        goto out;
+        return;
      }
      sens = ibs->sensors + cmd[2];
      IPMI_ADD_RSP_DATA(sens->reading);
@@ -1572,8 +1516,6 @@ static void get_sensor_reading(IPMIBmcSim *ibs,
      if (IPMI_SENSOR_IS_DISCRETE(sens)) {
          IPMI_ADD_RSP_DATA((sens->states >> 8) & 0xff);
      }
- out:
-    return;
  }
static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = {




reply via email to

[Prev in Thread] Current Thread [Next in Thread]