[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2] net: remove an assert call in eth_get_gso_type
Date: Wed, 21 Oct 2020 08:29:17 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1

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 
              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:
If this is some functionality we ought to implement but have
not, and so something will now be broken:
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
- 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

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()?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]