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

[Juergen Gross <jgross@xxxxxxxx>]

On 07.11.23 15:31, Jason Andryuk wrote:
> 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.
Same reasoning as for the write case: read() will do it for me.

> > +    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)).
> """
This is part of "only operations needed for Xenstore-stubdom are implemented."
For supporting Linux guests or other stubdoms directory reading must be added,
of course.

> > +    }
> > +    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.
Correct.

> > +    {
> > +        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?
If len == count there was no partial read, as this would have reduced len.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature