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.gong...@huawei.com wrote:
>>>> From: Gonglei <arei.gong...@huawei.com>
>>>>
>>>> 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 <arei.gong...@huawei.com>
>>>> ---
>>>> 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.
