[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 14/29] tools/xenlogd: add 9pfs read request support
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |