Re: [UNIKRAFT PATCH v2 14/15] plat/tap: Support tap_netdev_xmit

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

thanks a lot for this patch. There are a few issues in tap_write,
comments inline.

regards,
Hugo

On Tue, 2020-06-30 at 11:58 +0200, Sharan Santhanam wrote:
> Implement packet send on a tap device.
> 
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> ---
>  plat/drivers/include/tap/tap.h |  1 +
>  plat/drivers/tap/tap.c         | 18 +++++++++++++++---
>  plat/linuxu/tap_io.c           | 20 ++++++++++++++++++++
>  3 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/plat/drivers/include/tap/tap.h
> b/plat/drivers/include/tap/tap.h
> index 797a62e..648f9ef 100644
> --- a/plat/drivers/include/tap/tap.h
> +++ b/plat/drivers/include/tap/tap.h
> @@ -48,5 +48,6 @@ int tap_dev_configure(int fd, __u32 feature_flags,
> void *arg);
>  int tap_netif_configure(int fd, __u32 request, void *arg);
>  int tap_netif_create(void);
>  __ssz tap_read(int fd, void *buf, size_t count);
> +__ssz tap_write(int fd, const void *buf, size_t count);
>  
>  #endif /* __PLAT_DRV_TAP_H */
> diff --git a/plat/drivers/tap/tap.c b/plat/drivers/tap/tap.c
> index 998b9e8..505f0cc 100644
> --- a/plat/drivers/tap/tap.c
> +++ b/plat/drivers/tap/tap.c
> @@ -274,14 +274,13 @@ static int tap_netdev_recv(struct uk_netdev
> *dev,
>       rc = queue->alloc_rxpkts(queue->alloc_rxpkts_argp, &_pkt,
> 1);
>       if (rc == 0) {
>               uk_pr_err(DRIVER_NAME": Failed to allocate the
> memory\n");
> -             rc = -ENOMEM;
>               _pkt = NULL;
> -             goto err_exit;
> +             rc = UK_NETDEV_STATUS_UNDERRUN |
> UK_NETDEV_STATUS_MORE;
> +             return 0;
>       }
>       uk_pr_debug(DRIVER_NAME": Receiving on interface %s(%d)
> %p(%d)\n",
>                   tdev->name, queue->fd, _pkt->data, _pkt->len);
>       rc = tap_read(queue->fd, _pkt->data, _pkt->len);
> -
>       if (rc > 0) {
>               uk_pr_debug(DRIVER_NAME": Recv pkt size: %d\n", rc);
>               /* Setting the length of the packet */
> @@ -313,10 +312,23 @@ static int tap_netdev_xmit(struct uk_netdev
> *dev,
>                          struct uk_netbuf *pkt)
>  {
>       int rc = -EINVAL;
> +     struct tap_net_dev *tdev __unused;
>  
>       UK_ASSERT(dev);
>       UK_ASSERT(queue && pkt);
>  
> +     tdev = to_tapnetdev(dev);
> +
> +     rc = tap_write(queue->fd, pkt->data, pkt->len);
> +     if (rc > 0) {
> +             uk_pr_info(DRIVER_NAME": Send packet of size %d\n",
> rc);
> +             uk_netbuf_free(pkt);
> +             rc = UK_NETDEV_STATUS_SUCCESS |
> UK_NETDEV_STATUS_MORE;
> +     } else if (rc == -EWOULDBLOCK || rc == -EAGAIN) {
> +             uk_pr_info(DRIVER_NAME": The send queue is full\n");
> +             rc = UK_NETDEV_STATUS_UNDERRUN;
> +     }
> +
>       return rc;
>  }
>  
> diff --git a/plat/linuxu/tap_io.c b/plat/linuxu/tap_io.c
> index 01c9bff..030c192 100644
> --- a/plat/linuxu/tap_io.c
> +++ b/plat/linuxu/tap_io.c
> @@ -123,6 +123,26 @@ ssize_t tap_read(int fd, void *buf, size_t
> count)
>       return rc;
>  }
>  
> +ssize_t tap_write(int fd, const void *buf, size_t count)
> +{
> +     ssize_t rc = -EINTR;
> +     int written = 0;

This is a corner case, but `written` might overflow with very large
values of `count`. `size_t written` would be safer.

> +
> +     while (count > 0) {
> +             rc = sys_write(fd, buf, count);

We should write `buf + written` here. If the first sys_write fails, we
will attempt to write the first `count - rc` bytes of the buffer (which
have already been successfully written) instead of the last `count -
rc` bytes.

> +             if (rc == -EINTR)
> +                     continue;
> +             else if (rc < 0) {
> +                     uk_pr_err("Failed(%ld) to write to the tap
> device\n",
> +                               rc);
> +                     return rc;
> +             }
> +             count -= rc;
> +             written += rc;
> +     }
> +     return written;

Missing cast here?

> +}
> +
>  int tap_close(int fd)
>  {
>       return sys_close(fd);