Re: [PATCH v2] net: remove an assert call in eth_get_gso_type

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Peter, Stefan,

On 10/20/20 5:05 PM, Peter Maydell wrote:
> On Tue, 20 Oct 2020 at 15:05, P J P <ppandit@redhat.com> wrote:
>>
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> eth_get_gso_type() routine returns segmentation offload type based on
>> L3 protocol type. It calls g_assert_not_reached if L3 protocol is
>> unknown, making the following return statement unreachable. Remove the
>> g_assert call, as it maybe triggered by a guest user.
>>
>> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>   net/eth.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> Update v2: add qemu_log()
>>    -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html
>>
>> diff --git a/net/eth.c b/net/eth.c
>> index 0c1d413ee2..fd76e349eb 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -16,6 +16,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>   #include "net/eth.h"
>>   #include "net/checksum.h"
>>   #include "net/tap.h"
>> @@ -71,9 +72,7 @@ eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t l4proto)
>>               return VIRTIO_NET_HDR_GSO_TCPV6 | ecn_state;
>>           }
>>       }
>> -
>> -    /* Unsupported offload */
>> -    g_assert_not_reached();
>> +    qemu_log("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);
> 
> It's generally not a good idea to use qemu_log() without a
> particular mask, as then it will get printed if the user turns
> on any logging but not otherwise.
> 
> If the guest must have done something wrong to get us here:
>   use LOG_GUEST_ERROR
> If this is some functionality we ought to implement but have
> not, and so something will now be broken:
>   use LOG_UNIMP
> If the fallback for what happens in this situation is fine,
> and maybe it's just suboptimal performance, or an unusual
> case that might be interesting to know about but which
> we're handling within the spec:
>   consider a tracepoint instead

During the last 2 years I've been sending patches touching
various QEMU areas, but I never used qemu_log(). I always
used:
- qemu_log_mask(LOG_GUEST_ERROR/LOG_UNIMP, ...
- error_report/warn_report from "qemu/error-report.h"
- error_setg* from "qapi/error.h"
- trace events

$ git grep qemu_log\( | wc -l
661

This function seems used mostly by very old code.

It is declared in "qemu/log-for-trace.h" which looks like
an internal API.

Should we add a checkpatch rule to refuse new uses of qemu_log()?

Regards,

Phil.