Re: [PATCH v2 7/7] Mini-OS: add read and write support to 9pfsfront

[Juergen Gross <jgross@xxxxxxxx>]

On 10.02.23 19:59, Samuel Thibault wrote:
> Juergen Gross, le ven. 10 févr. 2023 11:46:28 +0100, a ecrit:
> > +    while ( len )
> > +    {
> > +        count = len;
> > +        if ( count > dev->msize_max - 24 )
> > +            count = dev->msize_max - 24;
> 24 should be detailed, to include e.g. sizeof(p9_header) and the sum of
> sizes of the fields (I'm surprised that it's the same 24 for read and
> write, notably).
It was just a common value chosen to be large enough. I even considered
to limit to a power of 2 or a multiple of PAGE_SIZE instead.

Independent from the chosen new value of count I agree, that at least the
test for count needing to be modified should be done with an exact value
for the upper limit.

> > +        send_9p(dev, req, "ULU", fid, offset, count);
> > +        rcv_9p(dev, req, "D", &count, data);
> > +
> > +        if ( !count )
> > +            break;
> > +        if ( req->result )
> > +        {
> > +            ret = -1;
> > +            errno = EIO;
> I'd say log req->result?
Okay.

> > +            break;
> > +        }
> > +        ret += count;
> > +        offset += count;
> > +        data += count;
> > +        len -= count;
> > +    }
> > +
> > +    put_free_req(dev, req);
> > +
> > +    return ret;
> > +}
> > +
> > +static int p9_write(struct dev_9pfs *dev, uint32_t fid, uint64_t offset,
> > +                    const uint8_t *data, uint32_t len)
> > +{
> > +    struct req *req = get_free_req(dev);
> > +    int ret = 0;
> > +    uint32_t count;
> > +
> > +    if ( !req )
> > +    {
> > +        errno = EAGAIN;
> > +        return -1;
> > +    }
> > +    req->cmd = P9_CMD_WRITE;
> > +
> > +    while ( len )
> > +    {
> > +        count = len;
> > +        if ( count > dev->msize_max - 24 )
> > +            count = dev->msize_max - 24;
> Same here.
Yes.

> > +        send_9p(dev, req, "ULD", fid, offset, count, data);
> > +        rcv_9p(dev, req, "U", &count);
> > +        if ( req->result )
> > +        {
> > +            ret = -1;
> > +            errno = EIO;
> Same here.
Yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature