On 5 April 2017 at 00:23, Bill Fischofer <[email protected]> wrote: > On Tue, Apr 4, 2017 at 5:20 PM, Bill Fischofer > <[email protected]> wrote: >> This is clearly orthogonal to this patch series. Ideally you should >> (a) Create a Bug to represent this, (b) Post the fix patch noting the >> Bug URL in the commit log, and (c) update the Bug entry with the URL >> of the patch that fixes this bug. >> >> On Tue, Apr 4, 2017 at 1:47 PM, Brian Brooks <[email protected]> wrote: >>> Signed-off-by: Kevin Wang <[email protected]> >>> Reviewed-by: Ola Liljedahl <[email protected]> >>> Reviewed-by: Brian Brooks <[email protected]> >>> --- >>> platform/linux-generic/pktio/loop.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/platform/linux-generic/pktio/loop.c >>> b/platform/linux-generic/pktio/loop.c >>> index 70962839..49d8a211 100644 >>> --- a/platform/linux-generic/pktio/loop.c >>> +++ b/platform/linux-generic/pktio/loop.c >>> @@ -176,6 +176,7 @@ static int loopback_send(pktio_entry_t *pktio_entry, >>> int index ODP_UNUSED, >>> pktio_entry->s.stats.out_octets += bytes; >>> } else { >>> ODP_DBG("queue enqueue failed %i\n", ret); >>> + odp_ticketlock_unlock(&pktio_entry->s.txl); >>> return -1; >> >> A better fix to this is to just delete the return -1 since that will >> result in the following unlock being executed and ret being returned >> as the return code from this routine. I thought we had a coding style that says we shall embrace multiple exits from a function. At least it looks like that.
However I think that this a very bad coding style because it creates much more or less redundant code where this typ of bug (forget to undo some side effect performed earlier in the function) is basically guaranteed to happen. Multiple exits with redundant code segments are also fragile when you start changing the code, you easily forget to update some exit path but this is unlikely to be caught by testing, it is very difficult to achieve 100% code coverage, including all error handling. Especially error handling is undertested, many code paths are very implementation specific and not amenable to black box testing. One project I worked with in the past (OSEck RTOS) did extensive white box testing, making additions or changes was very cumbersome but code quality was very high. > > Correction, change return -1 to ret = -1 since the test on the if if > (ret > 0) and we want to return a negative if ret = 0 from > odp_enqueue_multi(). > >> >>> } >>> >>> -- >>> 2.12.2 >>>
