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
