Re: [UNIKRAFT PATCH v2 1/9] lib/uk9p: Add new serialization mechanism

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>

On 5/9/20 7:44 PM, Cristian Banu wrote:
> This patch implements serialization in a type-safe way: the compiler can
> issue warnings if the data types don't match.  Previously the types were
> hidden behind a va_list and thus unchecked.
> 
> Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
> ---
>  lib/uk9p/include/uk/9preq.h | 184 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 184 insertions(+)
> 
> diff --git a/lib/uk9p/include/uk/9preq.h b/lib/uk9p/include/uk/9preq.h
> index ee4d2af..70c1b03 100644
> --- a/lib/uk9p/include/uk/9preq.h
> +++ b/lib/uk9p/include/uk/9preq.h
> @@ -41,6 +41,7 @@
>  #include <uk/essentials.h>
>  #include <uk/list.h>
>  #include <uk/refcount.h>
> +#include <uk/9p_core.h>
>  #if CONFIG_LIBUKSCHED
>  #include <uk/wait_types.h>
>  #endif
> @@ -293,6 +294,189 @@ int uk_9preq_waitreply(struct uk_9preq *req);
>   */
>  int uk_9preq_error(struct uk_9preq *req);
>  
> +/*
> + * The following family of serialization and deserialization functions
> + * are used for writing the 'base' types the 9p protocol supports.
> + *
> + * These are defined in the header for better performance by not necessarily
> + * incurring a function call penalty if called from outside uk9p.
> + *
> + * Provided functions:
> + * - uk_9preq_{read,write}buf
> + * - uk_9preq_{read,write}8
> + * - uk_9preq_{read,write}16
> + * - uk_9preq_{read,write}32
> + * - uk_9preq_{read,write}64
> + * - uk_9preq_{read,write}qid
> + * - uk_9preq_{read,write}str
> + * - uk_9preq_{read,write}stat
> + *
> + * For qid, str and stat, read and write always take a pointer.
> + * For all other types, write takes the argument by value.
> + *
> + * Possible return values:
> + * - 0: Operation successful.
> + * - (-ENOBUFS): End of buffer reached.
> + */
> +
> +static inline int uk_9preq_writebuf(struct uk_9preq *req, const void *buf,
> +             uint32_t size)
> +{
> +     if (req->xmit.offset + size > req->xmit.size)
> +             return -ENOBUFS;
> +
> +     memcpy((char *)req->xmit.buf + req->xmit.offset, buf, size);
> +     req->xmit.offset += size;
> +     return 0;
> +}
> +
> +static inline int uk_9preq_readbuf(struct uk_9preq *req, void *buf,
> +             uint32_t size)
> +{
> +     if (req->recv.offset + size > req->recv.size)
> +             return -ENOBUFS;
> +
> +     memcpy(buf, (char *)req->recv.buf + req->recv.offset, size);
> +     req->recv.offset += size;
> +     return 0;
> +}
> +
> +#define _UK_9PREQ_DEFINE_WRITE_FN(name, ctype) \
> +static inline int uk_9preq_##name(struct uk_9preq *req, ctype val) \
> +{ \
> +     return uk_9preq_writebuf(req, &val, sizeof(val)); \
> +}
> +
> +_UK_9PREQ_DEFINE_WRITE_FN(write8, uint8_t)
> +_UK_9PREQ_DEFINE_WRITE_FN(write16, uint16_t)
> +_UK_9PREQ_DEFINE_WRITE_FN(write32, uint32_t)
> +_UK_9PREQ_DEFINE_WRITE_FN(write64, uint64_t)
> +
> +#undef _UK_9PREQ_DEFINE_WRITE_FN
> +
> +static inline int uk_9preq_writeqid(struct uk_9preq *req, struct uk_9p_qid 
> *val)
> +{
> +     int rc;
> +
> +     if ((rc = uk_9preq_write8(req, val->type)) ||
> +             (rc = uk_9preq_write32(req, val->version)) ||
> +             (rc = uk_9preq_write64(req, val->path)))
> +             return rc;
> +
> +     return 0;
> +}
> +
> +static inline int uk_9preq_writestr(struct uk_9preq *req, struct uk_9p_str 
> *val)
> +{
> +     int rc;
> +
> +     if ((rc = uk_9preq_write16(req, val->size)) ||
> +             (rc = uk_9preq_writebuf(req, val->data, val->size)))
> +             return rc;
> +
> +     return 0;
> +}
> +
> +static inline int uk_9preq_writestat(struct uk_9preq *req,
> +             struct uk_9p_stat *val)
> +{
> +     int rc;
> +
> +     val->size = 61;
> +     val->size += val->name.size;
> +     val->size += val->uid.size;
> +     val->size += val->gid.size;
> +     val->size += val->muid.size;
> +     val->size += val->extension.size;
> +
> +     if ((rc = uk_9preq_write16(req, val->size)) ||
> +             (rc = uk_9preq_write16(req, val->type)) ||
> +             (rc = uk_9preq_write32(req, val->dev)) ||
> +             (rc = uk_9preq_writeqid(req, &val->qid)) ||
> +             (rc = uk_9preq_write32(req, val->mode)) ||
> +             (rc = uk_9preq_write32(req, val->atime)) ||
> +             (rc = uk_9preq_write32(req, val->mtime)) ||
> +             (rc = uk_9preq_write64(req, val->length)) ||
> +             (rc = uk_9preq_writestr(req, &val->name)) ||
> +             (rc = uk_9preq_writestr(req, &val->uid)) ||
> +             (rc = uk_9preq_writestr(req, &val->gid)) ||
> +             (rc = uk_9preq_writestr(req, &val->muid)) ||
> +             (rc = uk_9preq_writestr(req, &val->extension)) ||
> +             (rc = uk_9preq_write32(req, val->n_uid)) ||
> +             (rc = uk_9preq_write32(req, val->n_gid)) ||
> +             (rc = uk_9preq_write32(req, val->n_muid)))
> +             return rc;
> +
> +     return 0;
> +}
> +
> +#define _UK_9PREQ_DEFINE_READ_FN(name, ctype) \
> +static inline int uk_9preq_##name(struct uk_9preq *req, ctype * val) \
> +{ \
> +     return uk_9preq_readbuf(req, val, sizeof(*val)); \
> +}
> +
> +_UK_9PREQ_DEFINE_READ_FN(read8, uint8_t)
> +_UK_9PREQ_DEFINE_READ_FN(read16, uint16_t)
> +_UK_9PREQ_DEFINE_READ_FN(read32, uint32_t)
> +_UK_9PREQ_DEFINE_READ_FN(read64, uint64_t)
> +
> +#undef _UK_9PREQ_DEFINE_READ_FN
> +
> +static inline int uk_9preq_readqid(struct uk_9preq *req, struct uk_9p_qid 
> *val)
> +{
> +     int rc;
> +
> +     if ((rc = uk_9preq_read8(req, &val->type)) ||
> +             (rc = uk_9preq_read32(req, &val->version)) ||
> +             (rc = uk_9preq_read64(req, &val->path)))
> +             return rc;
> +
> +     return 0;
> +}
> +
> +static inline int uk_9preq_readstr(struct uk_9preq *req, struct uk_9p_str 
> *val)
> +{
> +     int rc;
> +
> +     if ((rc = uk_9preq_read16(req, &val->size)))
> +             return rc;
> +
> +     /* Optimized string read, does not allocate memory. */
> +     val->data = (char *)req->recv.buf + req->recv.offset;
> +     req->recv.offset += val->size;
> +     if (req->recv.offset > req->recv.size)
> +             return -ENOBUFS;
> +
> +     return 0;
> +}
> +
> +static inline int uk_9preq_readstat(struct uk_9preq *req,
> +             struct uk_9p_stat *val)
> +{
> +     int rc;
> +
> +     if ((rc = uk_9preq_read16(req, &val->size)) ||
> +             (rc = uk_9preq_read16(req, &val->type)) ||
> +             (rc = uk_9preq_read32(req, &val->dev)) ||
> +             (rc = uk_9preq_readqid(req, &val->qid)) ||
> +             (rc = uk_9preq_read32(req, &val->mode)) ||
> +             (rc = uk_9preq_read32(req, &val->atime)) ||
> +             (rc = uk_9preq_read32(req, &val->mtime)) ||
> +             (rc = uk_9preq_read64(req, &val->length)) ||
> +             (rc = uk_9preq_readstr(req, &val->name)) ||
> +             (rc = uk_9preq_readstr(req, &val->uid)) ||
> +             (rc = uk_9preq_readstr(req, &val->gid)) ||
> +             (rc = uk_9preq_readstr(req, &val->muid)) ||
> +             (rc = uk_9preq_readstr(req, &val->extension)) ||
> +             (rc = uk_9preq_read32(req, &val->n_uid)) ||
> +             (rc = uk_9preq_read32(req, &val->n_gid)) ||
> +             (rc = uk_9preq_read32(req, &val->n_muid)))
> +             return rc;
> +
> +     return 0;
> +}
> +
>  #ifdef __cplusplus
>  }
>  #endif
>