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

[Juergen Gross <jgross@xxxxxxxx>]

On 06.02.23 10:40, Samuel Thibault wrote:
> 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).
Okay, will add some more comments.

> > +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.
I can add a log message.

> > +        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.
Okay.

> > +        case 'Q':
> > +            qval = va_arg(ap, uint8_t *);
> > +            copy_bufs(&buf1, &buf2, &len1, &len2, qval, 13);
> I'd say make this 13 a macro.
Okay.

> > +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?
Hmm, yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature