Changeset 31215 in vbox

Timestamp:

Jul 29, 2010 2:20:04 PM (15 years ago)

Author:

vboxsync

Message:

Review notes.

File:

• trunk/src/VBox/Devices/Audio/DevCodec.cpp (modified) (9 diffs)

trunk/src/VBox/Devices/Audio/DevCodec.cpp

@@ -155 +155 @@
 {
     uint32_t *pu32Bparam = NULL;
+    //** @todo r=michaln: Missing NID bounds checking!
     PCODECNODE pNode = &pState->pNodes[CODEC_NID(cmd)];
     *pResp = 0;
@@ -184 +185 @@
 {
     Assert((CODEC_CAD(cmd) == pState->id));
+    //** @todo r=michaln: This is not bounds checked and may overflow the pNodes array!
     *pResp = pState->pNodes[CODEC_NID(cmd)].node.au32F00_param[cmd & CODEC_VERB_8BIT_DATA];
     return VINF_SUCCESS;
@@ -195 +197 @@
         *pResp = pState->pNodes[CODEC_NID(cmd)].adcmux.u32F01_param;
     else if (STAC9220_IS_DIGOUTPIN_CMD(cmd))
+        //** @todo r=michaln: Is that really u32F07, or should it be u32F01?
         *pResp = pState->pNodes[CODEC_NID(cmd)].digout.u32F07_param;
+    //** @todo r=michaln: Else what? We must always fill out *pResp!
     return VINF_SUCCESS;
 }
@@ -228 +232 @@
         *pResp = pState->pNodes[CODEC_NID(cmd)].cdnode.u32F07_param;
     else
+        //** @todo r=michaln: pResp must still be filled out
         AssertMsgFailed(("Unsupported"));
     return VINF_SUCCESS;
@@ -284 +289 @@
         pu32Reg = &pState->pNodes[CODEC_NID(cmd)].volumeKnob.u32F08_param;
     else
+        //** @todo r=michaln: This will crash in release builds! (pu32Reg will be NULL here)
         AssertMsgFailed(("unsupported operation %x on node: %x\n", CODEC_VERB_CMD8(cmd), CODEC_NID(cmd)));
     Assert(pu32Reg);
@@ -324 +330 @@
 {
     Assert((CODEC_CAD(cmd) == pState->id));
+    //** @todo r=michaln: Again, possible pNodes array overflow
     *pResp = *(uint32_t *)&pState->pNodes[CODEC_NID(cmd)].node.au8F02_param[cmd & CODEC_VERB_8BIT_DATA];
     return VINF_SUCCESS;
@@ -389 +396 @@
         *pResp = pState->pNodes[CODEC_NID(cmd)].afg.u32F20_param;
     }
+    //** @todo r=michaln: pResp must always be filled out
     return VINF_SUCCESS;
 }
@@ -458 +466 @@
     else
         *pResp = 0; /* STAC9220 6.20 6.13-6.18: no response supposed for this verb. */
+    //** @todo r=michaln: Is this intentional? I don't think we should always return 0?
     *pResp = 0;
     return VINF_SUCCESS;
@@ -525 +534 @@
     else if (STAC9220_IS_DIGINPIN_CMD(cmd))
         *pResp = pState->pNodes[CODEC_NID(cmd)].digin.u32F0c_param;
+    //** @todo r=michaln: Do we really always want to return zero?
     *pResp = 0;
     return VINF_SUCCESS;