Re: [Qemu-devel] [PATCH 2/6] rtl8139: g_malloc() can't fail, bury dead error handling

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> On Tue, 27 Jan 2015 17:38:27 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  hw/net/rtl8139.c | 14 --------------
>>  1 file changed, 14 deletions(-)
>> 
>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>> index 6fa9e0a..b55e438 100644
>> --- a/hw/net/rtl8139.c
>> +++ b/hw/net/rtl8139.c
>> @@ -2075,20 +2075,6 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>>                  "length to %d\n", txsize);
>>      }
>> 
>> -    if (!s->cplus_txbuffer)
>> -    {
>> -        /* out of memory */
>> -
>> -        DPRINTF("+++ C+ mode transmiter failed to reallocate %d bytes\n",
>> -            s->cplus_txbuffer_len);
>> -
>> -        /* update tally counter */
>> -        ++s->tally_counters.TxERR;
>> -        ++s->tally_counters.TxAbt;
>> -
>> -        return 0;
>> -    }
>> -
>>      /* append more data to the packet */
>> 
>>      DPRINTF("+++ C+ mode transmit reading %d bytes from host memory at "
>
> Wouldn't it be better to use g_try_malloc() here instead? If the code
> can handle OOM conditions, I think it's better to continue with a lost
> packet here than to shut down QEMU the hard way.
> (Also looking at the history of that file, the code originally used
> qemu_malloc() which could fail - but instead of being replaced by
> g_try_malloc(), it got replaced with g_malloc() instead which was
> maybe the wrong decision).

When allocating 64KiB[*] fails, then you're a dead process walking.

By checking for and recovering from such allocation failures, you can
try to limp on for a bit.  If your programmers have been perfectly
diligent, you'll limp on until you die in a non-recoverable allocation
failure.  More likely, your programmers have been human, and you'll die
an ugly death after some unchecked allocation or in some untested
recovery that doesn't actually work.

Our strategy is to use allocations that need checking only for large
sizes and where recovery is possible without crippling loss of
functionality.  Those are rare.  Gives us a chance to actually test
their recovery.

There's ample prededence for ditching untested allocation error recovery
in favour of simply terminating the process.

If you think recovery is worthwhile here, post a patch.  Ideally
extending tests/rtl8139-test.c to cover the error recovery.


[*] That's the value of s->cplus_txbuffer_len (apparent once you peel
off the obfuscation).