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

[Philippe Mathieu-Daudé]

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.