Re: [Minios-devel] [UNIKRAFT PATCH 6/7] lib/ukconsdev: Read/Write from console device

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

In general, I would call your write and read, transmit and receive. That 
would be more inline with how your named the rings/queues. Other from 
this, this patch looks okay.
On 19.06.19 15:35, Birlea Costin wrote:
> Signed-off-by: Birlea Costin <costin.birlea@xxxxxxxxx>
> ---
>   lib/ukconsdev/consdev.c                 | 30 ++++++++++++++++++++++++++++++
>   lib/ukconsdev/exportsyms.uk             |  2 ++
>   lib/ukconsdev/include/uk/consdev.h      | 30 ++++++++++++++++++++++++++++++
>   lib/ukconsdev/include/uk/consdev_core.h | 10 ++++++++++
>   4 files changed, 72 insertions(+)
>
> diff --git a/lib/ukconsdev/consdev.c b/lib/ukconsdev/consdev.c
> index 6c1e8185..b91b0a7f 100644
> --- a/lib/ukconsdev/consdev.c
> +++ b/lib/ukconsdev/consdev.c
> @@ -343,6 +343,34 @@ out:
>         return rc;
>   }
>   
> +unsigned int uk_consdev_write(struct uk_consdev *dev,
> +               char *buf, unsigned int len)
Maybe you want to declare the buffer as `const char *`. Your driver will 
not modify it, just copying the characters over (no zero-copy), right?
> +{
> +       UK_ASSERT(dev);
> +       UK_ASSERT(dev->_data);
> +       UK_ASSERT(dev->ops);
> +       UK_ASSERT(dev->ops->read);
> +
> +       if (dev->_data->state != UK_CONSDEV_RUNNING)
> +               return -EINVAL;
> +
> +       return dev->ops->write(dev, buf, len);
> +}
> +
> +unsigned int uk_consdev_read(struct uk_consdev *dev,
> +               char *buf, unsigned int len)
> +{
> +       UK_ASSERT(dev);
> +       UK_ASSERT(dev->_data);
> +       UK_ASSERT(dev->ops);
> +       UK_ASSERT(dev->ops->read);
> +
> +       if (dev->_data->state != UK_CONSDEV_RUNNING)
> +               return -EINVAL;
> +
> +       return dev->ops->read(dev, buf, len);
> +}
> +
>   unsigned int uk_consdev_count(void)
>   {
>         return (unsigned int) consdev_count;
> @@ -417,6 +445,8 @@ int uk_consdev_drv_register(struct uk_consdev *dev, struct 
> uk_alloc *a,
>         UK_ASSERT(dev->ops->rx_configure);
>         UK_ASSERT(dev->ops->tx_configure);
>         UK_ASSERT(dev->ops->release);
> +       UK_ASSERT(dev->ops->write);
> +       UK_ASSERT(dev->ops->read);
>         UK_ASSERT(dev->ops->start);
>         UK_ASSERT(dev->ops->stop);
>         UK_ASSERT(dev->ops->close);
> diff --git a/lib/ukconsdev/exportsyms.uk b/lib/ukconsdev/exportsyms.uk
> index dff1c331..4d39941d 100644
> --- a/lib/ukconsdev/exportsyms.uk
> +++ b/lib/ukconsdev/exportsyms.uk
> @@ -7,6 +7,8 @@ uk_consdev_tx_configure
>   uk_consdev_release
>   uk_consdev_start
>   uk_consdev_stop
> +uk_consdev_write
> +uk_consdev_read
>   uk_consdev_count
>   uk_consdev_get
>   uk_consdev_id_get
> diff --git a/lib/ukconsdev/include/uk/consdev.h 
> b/lib/ukconsdev/include/uk/consdev.h
> index 779ebdbc..9f2f0567 100644
> --- a/lib/ukconsdev/include/uk/consdev.h
> +++ b/lib/ukconsdev/include/uk/consdev.h
> @@ -219,6 +219,36 @@ int uk_consdev_start(struct uk_consdev *dev);
>   int uk_consdev_stop(struct uk_consdev *dev);
>   
>   /**
> + * @param dev
> + *  The Unikraft Console Device in running state.
> + * @param buf
> + *  Pointer from which to write on the ring.
> + * @param len
> + *  How many bytes to take from buf and write to ring.
> + * @return
> + *     - (0): Ring is full.
> + *     - (>0): Number of bytes written.
> + *     - (<0): Error code of the driver device, write failed.
> + */
> +unsigned int uk_consdev_write(struct uk_consdev *dev,
> +               char *buf, unsigned int len);
> +
> +/**
> + * @param dev
> + *  The Unikraft Console Device in running state.
> + * @param buf
> + *  Pointer where to put bytes read from ring.
> + * @param len
> + *  How many bytes to read from ring.
> + * @return
> + *     - (0): Ring is empty.
> + *     - (>0): Number of bytes read.
> + *     - (<0): Error code of the driver device, read failed.
> + */
> +unsigned int uk_consdev_read(struct uk_consdev *dev,
> +               char *buf, unsigned int len);
> +
> +/**
>    * Get the number of available Unikraft Console devices.
>    *
>    * @return
> diff --git a/lib/ukconsdev/include/uk/consdev_core.h 
> b/lib/ukconsdev/include/uk/consdev_core.h
> index 41f2ea3b..6c675fa7 100644
> --- a/lib/ukconsdev/include/uk/consdev_core.h
> +++ b/lib/ukconsdev/include/uk/consdev_core.h
> @@ -176,6 +176,14 @@ typedef int (*uk_consdev_start_t)(struct uk_consdev *dev);
>   /** Driver callback type to stop a running Unikraft console device. */
>   typedef int (*uk_consdev_stop_t)(struct uk_consdev *dev);
>   
> +/** Driver callback type to write to a running Unikraft console device. */
> +typedef unsigned int (*uk_consdev_write_t)(struct uk_consdev *dev,
> +               char *buf, unsigned int len);
> +
> +/** Driver callback type to read to a running Unikraft console device. */
> +typedef unsigned int (*uk_consdev_read_t)(struct uk_consdev *dev,
> +               char *buf, unsigned int len);
> +
>   /** Driver callback type to close an Unikraft console device. */
>   typedef void (*uk_consdev_close_t)(struct uk_consdev *dev);
>   
> @@ -189,6 +197,8 @@ struct uk_consdev_ops {
>         uk_consdev_tx_configure_t           tx_configure;
>         uk_consdev_start_t                  start;
>         uk_consdev_stop_t                   stop;
> +       uk_consdev_write_t                  write;
> +       uk_consdev_read_t                   read;
>         uk_consdev_close_t                  close;
>   };
>   
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel