| Message ID | 20231101093325.30302-15-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools: enable xenstore-stubdom to use 9pfs | expand |
On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross <jgross@suse.com> wrote: > > Add the read request of the 9pfs protocol. > > For now support only reading plain files (no directories). > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 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;
On 07.11.23 15:31, Jason Andryuk wrote: > On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross <jgross@suse.com> wrote: >> >> Add the read request of the 9pfs protocol. >> >> For now support only reading plain files (no directories). >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> 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
On Tue, Nov 7, 2023 at 9:49 AM Juergen Gross <jgross@suse.com> wrote: > > On 07.11.23 15:31, Jason Andryuk wrote: > > On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross <jgross@suse.com> wrote: > >> > >> Add the read request of the 9pfs protocol. > >> > >> For now support only reading plain files (no directories). > >> > >> Signed-off-by: Juergen Gross <jgross@suse.com> > >> --- > >> 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@gmail.com> Regards, Jason
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 @@ -32,6 +32,7 @@ #define P9_CMD_WALK 110 #define P9_CMD_OPEN 112 #define P9_CMD_CREATE 114 +#define P9_CMD_READ 116 #define P9_CMD_WRITE 118 #define P9_CMD_CLUNK 120 #define P9_CMD_STAT 124 @@ -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; + } + + if ( fidp->isdir ) + { + p9_error(device, hdr->tag, EOPNOTSUPP); + return; + } + else + { + 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 ) + { + 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; @@ -1228,6 +1284,10 @@ void *io_thread(void *arg) p9_create(device, &hdr); break; + case P9_CMD_READ: + p9_read(device, &hdr); + break; + case P9_CMD_WRITE: p9_write(device, &hdr); break;
Add the read request of the 9pfs protocol. For now support only reading plain files (no directories). Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenlogd/io.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)