From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46043) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrMW1-0005VK-EV for qemu-devel@nongnu.org; Thu, 20 Nov 2014 02:51:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XrMVw-0008MF-Jw for qemu-devel@nongnu.org; Thu, 20 Nov 2014 02:50:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36734) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XrMVw-0008M8-Bz for qemu-devel@nongnu.org; Thu, 20 Nov 2014 02:50:52 -0500 Message-ID: <546D9D50.4000500@redhat.com> Date: Thu, 20 Nov 2014 15:50:40 +0800 From: Jason Wang MIME-Version: 1.0 References: <1416463034-8264-1-git-send-email-arei.gonglei@huawei.com> <1416463034-8264-5-git-send-email-arei.gonglei@huawei.com> <546D8A48.8040502@redhat.com> <546D905A.5020306@redhat.com> <546D9449.5020107@huawei.com> In-Reply-To: <546D9449.5020107@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei Cc: Paolo Bonzini , "qemu-devel@nongnu.org" , "stefanha@redhat.com" , "Huangpeng (Peter)" On 11/20/2014 03:12 PM, Gonglei wrote: > On 2014/11/20 14:55, Jason Wang wrote: > >> On 11/20/2014 02:29 PM, Paolo Bonzini wrote: >>> On 20/11/2014 06:57, arei.gonglei@huawei.com wrote: >>>> From: Gonglei >>>> >>>> Coverity spot: >>>> Assigning: iov = struct iovec [3]({{buf, 12UL}, >>>> {(void *)dot1q_buf, 4UL}, >>>> {buf + 12, size - 12}}) >>>> (address of temporary variable of type struct iovec [3]). >>>> out_of_scope: Temporary variable of type struct iovec [3] goes out of scope. >>>> >>>> Pointer to local outside scope (RETURN_LOCAL) >>>> use_invalid: >>>> Using iov, which points to an out-of-scope temporary variable of type struct iovec [3]. >>>> >>>> Signed-off-by: Gonglei >>>> --- >>>> hw/net/rtl8139.c | 36 ++++++++++++------------------------ >>>> 1 file changed, 12 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c >>>> index 8b8a1b1..426171b 100644 >>>> --- a/hw/net/rtl8139.c >>>> +++ b/hw/net/rtl8139.c >>>> @@ -1775,6 +1775,8 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, >>>> int do_interrupt, const uint8_t *dot1q_buf) >>>> { >>>> struct iovec *iov = NULL; >>>> + size_t buf2_size; >>>> + uint8_t *buf2 = NULL; >>>> >>>> if (!size) >>>> { >>>> @@ -1789,35 +1791,21 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, >>>> { .iov_base = buf + ETHER_ADDR_LEN * 2, >>>> .iov_len = size - ETHER_ADDR_LEN * 2 }, >>>> }; >>>> - } >>>> >>>> - if (TxLoopBack == (s->TxConfig & TxLoopBack)) >>>> - { >>>> - size_t buf2_size; >>>> - uint8_t *buf2; >>>> - >>>> - if (iov) { >>>> - buf2_size = iov_size(iov, 3); >>>> - buf2 = g_malloc(buf2_size); >>>> - iov_to_buf(iov, 3, 0, buf2, buf2_size); >>>> - buf = buf2; >>>> - } >>>> + buf2_size = iov_size(iov, 3); >>>> + buf2 = g_malloc(buf2_size); >>>> + iov_to_buf(iov, 3, 0, buf2, buf2_size); >>>> + buf = buf2; >>>> + } >>> This makes rtl8139 even slower than it is for the vlantag case, using a >>> bounce buffer for every packet. Perhaps another solution could be to do > Indeed. Your approach is better. :) > >>> struct iovec *iov = NULL; >>> struct iovec vlan_iov[3]; >>> >>> ... >>> if (dot1q_buf && size >= ETHER_ADDR_LEN * 2) { >>> ... >>> memcpy(vlan_iov, &(struct iovec[3]) { >>> ... >>> }, sizeof(vlan_iov)); >>> iov = vlan_iov; >>> } >>> >>> (I think "vlan_iov = (struct iovec[3]) { ... };" does not work, > Yes. the same reason with the original issue. > >>> but I >>> may be wrong). >>> >>> Stefan, what do you think? >>> >>> Paolo >>> >>> >> Maybe just initialize iov unconditionally at the beginning and check >> dot1q_buf instead of iov for the rest of the functions. (Need deal with >> size < ETHER_ADDR_LEN * 2) > More complicated, because we can't initialize iov when > "size < ETHER_ADDR_LEN * 2". > > Best regards, > -Gonglei > Probably not: you can just do something like: if (dot1q_buf && size < ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows.