Re: [Qemu-devel] [PATCH V8 4/5] Adding packet abstraction for VMWARE network devices

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Dec 07, 2012 at 01:15:08PM +0200, Dmitry Fleytman wrote:
> +void vmxnet_tx_pkt_uninit(struct VmxnetTxPkt *pkt)
> +{
> +    if (pkt) {
> +        if (pkt->vec) {
> +            g_free(pkt->vec);
> +        }
> +
> +        if (pkt->raw) {
> +            g_free(pkt->raw);
> +        }

g_free(NULL) is a nop so the conditional is not necessary.

> +static size_t vmxnet_tx_pkt_do_sw_fragmentation(struct VmxnetTxPkt *pkt,
> +    NetClientState *nc)
> +{
> +    struct iovec fragment[VMXNET_MAX_FRAG_SG_LIST];
> +    size_t fragment_len = 0;
> +    bool more_frags = false;
> +    /* some poiners for shorter code */
> +    void *l2_iov_base, *l3_iov_base;
> +    size_t l2_iov_len, l3_iov_len;
> +    int src_idx =  VMXNET_TX_PKT_PL_START_FRAG, dst_idx;
> +    size_t src_offset = 0;
> +    size_t bytes_sent = 0;
> +    size_t fragment_offset = 0;
> +
> +    l2_iov_base = pkt->vec[VMXNET_TX_PKT_L2HDR_FRAG].iov_base;
> +    l2_iov_len = pkt->vec[VMXNET_TX_PKT_L2HDR_FRAG].iov_len;
> +    l3_iov_base = pkt->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_base;
> +    l3_iov_len = pkt->vec[VMXNET_TX_PKT_L3HDR_FRAG].iov_len;
> +
> +    /* Copy headers */
> +    fragment[VMXNET_TX_PKT_FRAGMENT_L2_HDR_POS].iov_base = l2_iov_base;
> +    fragment[VMXNET_TX_PKT_FRAGMENT_L2_HDR_POS].iov_len = l2_iov_len;
> +    fragment[VMXNET_TX_PKT_FRAGMENT_L3_HDR_POS].iov_base = l3_iov_base;
> +    fragment[VMXNET_TX_PKT_FRAGMENT_L3_HDR_POS].iov_len = l3_iov_len;
> +
> +
> +    /* Put as much data as possible and send */
> +    do {
> +        fragment_len = vmxnet_tx_pkt_fetch_fragment(pkt, &src_idx, &src_offset,
> +            fragment, &dst_idx);
> +
> +        more_frags = (fragment_offset + fragment_len < pkt->payload_len);
> +
> +        eth_setup_ip4_fragmentation(l2_iov_base, l2_iov_len, l3_iov_base,
> +            l3_iov_len, fragment_len, fragment_offset, more_frags);
> +
> +        eth_fix_ip4_checksum(l3_iov_base, l3_iov_len);
> +
> +        bytes_sent += qemu_sendv_packet(nc, fragment, dst_idx);

The return values from qemu_sendv_packet() are:

Positive - Number of bytes successfully sent
0 - Packet queued for deferred transmission
Negative - Error

Accumulating bytes_sent is not safe against negative return values and
also questionable for 0 return values.

Stefan