Re: [PATCH 5/7] Mini-OS: add 9pfs transport layer

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le ven. 03 févr. 2023 10:18:07 +0100, a ecrit:
> +/*
> + * Using an opportunistic approach for receiving data: in case multiple
> + * requests are outstanding (which is very unlikely), we nevertheless need
> + * to consume all data available until we reach the desired request.
> + * For requests other than the one we are waiting for, we link the complete
> + * data to the request via an intermediate buffer. For our own request we can
> + * omit that buffer and directly fill the caller provided variables.
> + */

This documents the "why" which is very welcome, but does not document
what the function does exactly (AIUI, copy from buf1+buf2 into target up
to len bytes, and update buf/len accordingly).

> +static void copy_bufs(unsigned char **buf1, unsigned char **buf2,
> +                      unsigned int *len1, unsigned int *len2,
> +                      void *target, unsigned int len)
> +{
> +    if ( len <= *len1 )
> +    {
> +        memcpy(target, *buf1, len);
> +        *buf1 += len;
> +        *len1 -= len;
> +    }
> +    else
> +    {
> +        memcpy(target, *buf1, *len1);
> +        target = (char *)target + *len1;
> +        len -= *len1;
> +        *buf1 = *buf2;
> +        *len1 = *len2;
> +        *buf2 = NULL;
> +        *len2 = 0;
> +        if ( len > *len1 )
> +            len = *len1;

But then this is a short copy, don't we need to error out, or at least
log something? Normally the other end is not supposed to push data
incrementaly, but better at least catch such misprogramming.

> +        memcpy(target, *buf1, *len1);
> +    }
> +}
> +
> +static void rcv_9p_copy(struct dev_9pfs *dev, struct req *req,
> +                        struct p9_header *hdr, const char *fmt, va_list ap)

Please document what this helper function does (at the very least which
way the copy happens). The hdr != NULL vs == NULL notably needs to be
explained.

> +        case 'Q':
> +            qval = va_arg(ap, uint8_t *);
> +            copy_bufs(&buf1, &buf2, &len1, &len2, qval, 13);

I'd say make this 13 a macro.


> +static void rcv_9p(struct dev_9pfs *dev, struct req *req, const char *fmt, 
> ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +
> +    down(&dev->ring_in_sem);
> +
> +    while ( !rcv_9p_one(dev, req, fmt, ap) );
> +
> +    rmb(); /* Read all data before updating ring index. */
> +    dev->intf->in_cons = dev->cons_pvt_in;

Shouldn't there be an event notification after updating the consumption
index?

> +    up(&dev->ring_in_sem);
> +
> +    va_end(ap);
> +}

Samuel