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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Nov 7, 2023 at 9:49 AM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> 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.

Right.  I found it confusing since ret > 0 is not an error.  As you
wrote, len != count in that case though.

Preferably with ret < 0:
Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Regards,
Jason