Re: [PATCH 14/29] tools/xenlogd: add 9pfs read request support

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> Add the read request of the 9pfs protocol.
>
> For now support only reading plain files (no directories).
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  tools/xenlogd/io.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index 6b4692ca67..b3f9f96bcc 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c

> @@ -1011,6 +1012,61 @@ static void p9_create(device *device, struct p9_header 
> *hdr)
>      fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", &qid, &iounit);
>  }
>
> +static void p9_read(device *device, struct p9_header *hdr)
> +{
> +    uint32_t fid;
> +    uint64_t off;
> +    unsigned int len;
> +    uint32_t count;
> +    void *buf;
> +    struct p9_fid *fidp;
> +    int ret;
> +
> +    ret = fill_data(device, "ULU", &fid, &off, &count);
> +    if ( ret != 3 )
> +    {
> +        p9_error(device, hdr->tag, EINVAL);
> +        return;
> +    }
> +
> +    fidp = find_fid(device, fid);
> +    if ( !fidp || !fidp->opened )
> +    {
> +        p9_error(device, hdr->tag, EBADF);
> +        return;
> +    }
> +

I think you want to mode check here for readability.

> +    if ( fidp->isdir )
> +    {
> +        p9_error(device, hdr->tag, EOPNOTSUPP);
> +        return;

Hmmm, 9P2000.u supports read on a dir.
https://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor30
"""
For directories, read returns an integral number of direc- tory
entries exactly as in stat (see stat(5)), one for each member of the
directory. The read request message must have offset equal to zero or
the value of offset in the previous read on the directory, plus the
number of bytes returned in the previous read. In other words, seeking
other than to the beginning is illegal in a directory (see seek(2)).
"""

> +    }
> +    else

Since the above is a return, maybe remove the else and un-indent?
Though maybe you don't want to do that if you want to implement read
on a dir.

> +    {
> +        len = count;
> +        buf = device->buffer + sizeof(*hdr) + sizeof(uint32_t);
> +
> +        while ( len != 0 )
> +        {
> +            ret = pread(fidp->fd, buf, len, off);
> +            if ( ret <= 0 )
> +                break;
> +            len -= ret;
> +            buf += ret;
> +            off += ret;
> +        }
> +        if ( ret && len == count )

This seems wrong to me - should this be ( ret < 0 && len == count ) to
indicate an error on the first pread?  Any partial reads would still
return their data?

Regards,
Jason

> +        {
> +            p9_error(device, hdr->tag, errno);
> +            return;
> +        }
> +
> +        buf = device->buffer + sizeof(*hdr) + sizeof(uint32_t);
> +        len = count - len;
> +        fill_buffer(device, hdr->cmd + 1, hdr->tag, "D", &len, buf);
> +    }
> +}
> +
>  static void p9_write(device *device, struct p9_header *hdr)
>  {
>      uint32_t fid;