| Message ID | 20231101093325.30302-6-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 6:56 AM Juergen Gross <jgross@suse.com> wrote: > > Add support for generation a 9pfs protocol response via a format based > approach. > > Strings are stored in a per device string buffer and they are > referenced via their offset in this buffer. This allows to avoid > having to dynamically allocate memory for each single string. > > As a first user of the response handling add a generic p9_error() > function which will be used to return any error to the client. > > Add all format parsing variants in order to avoid additional code churn > later when adding the users of those variants. Prepare a special case > for the "read" case already (format character 'D'): in order to avoid > adding another buffer for read data support doing the read I/O directly > into the response buffer. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c > index 590d06e906..5a06f72338 100644 > --- a/tools/xenlogd/io.c > +++ b/tools/xenlogd/io.c > @@ -101,6 +112,172 @@ static bool io_work_pending(device *device) > : ring_out_data(device); > } > > +static void fmt_err(const char *fmt) > +{ > + syslog(LOG_CRIT, "illegal format %s passed to fill_buffer()", fmt); > + exit(1); > +} > + > +/* > + * Fill buffer with response data. > + * fmt is a sequence of format characters. Supported characters are: > + * a: an array (2 bytes number of elements + the following format as elements) > + * The number of elements is passed in the first unsigned int parameter, the > + * next parameter is a pointer to an array of elements as denoted by the next > + * format character. > + * b: 2 byte unsigned integer > + * The parameter is a pointer to a uint16_t value > + * D: Data blob (4 byte length + <length> bytes) > + * 2 parameters are consumed, first an unsigned int for the length, then a > + * pointer to the first uint8_t value. > + * No array support. > + * L: 8 byte unsigned integer > + * The parameter is a pointer to a uint64_t value > + * Q: Qid (struct p9_qid) > + * S: String (2 byte length + <length> characters) > + * The length is obtained via strlen() of the parameter, being a pointer > + * to the first character of the string > + * U: 4 byte unsigned integer > + * The parameter is a pointer to a uint32_t value > + */ > +static void fill_buffer(device *device, uint8_t cmd, uint16_t tag, > + const char *fmt, ...) > +{ > + struct p9_header *hdr = device->buffer; > + void *data = hdr + 1; > + const char *f; > + const void *par; > + const char *str_val; > + const struct p9_qid *qid; > + unsigned int len; > + va_list ap; > + unsigned int array_sz = 0; > + unsigned int elem_sz = 0; > + > + hdr->cmd = cmd; > + hdr->tag = tag; > + > + va_start(ap, fmt); > + > + for ( f = fmt; *f; f++ ) > + { > + if ( !array_sz ) > + par = va_arg(ap, const void *); > + else > + { > + par += elem_sz; > + array_sz--; > + } > + > + switch ( *f ) > + { > + case 'a': > + f++; > + if ( !*f || array_sz ) > + fmt_err(fmt); > + array_sz = *(const unsigned int *)par; > + *(__packed uint16_t *)data = array_sz; Is it worth checking that array_sz doesn't exceed 0xffff? > + data += sizeof(uint16_t); > + par = va_arg(ap, const void *); > + elem_sz = 0; > + break; > + > + case 'u': > + *(__packed uint16_t *)data = *(const uint16_t *)par; > + elem_sz = sizeof(uint16_t); > + data += sizeof(uint16_t); > + break; > + > + case 'D': > + if ( array_sz ) > + fmt_err(fmt); > + len = *(const unsigned int *)par; > + *(__packed uint32_t *)data = len; > + data += sizeof(uint32_t); > + par = va_arg(ap, const void *); > + if ( data != par ) > + memcpy(data, par, len); > + data += len; > + break; > + > + case 'L': > + *(__packed uint64_t *)data = *(const uint64_t *)par; > + elem_sz = sizeof(uint64_t); > + data += sizeof(uint64_t); > + break; > + > + case 'Q': > + qid = par; > + elem_sz = sizeof(*qid); > + *(uint8_t *)data = qid->type; > + data += sizeof(uint8_t); > + *(__packed uint32_t *)data = qid->version; > + data += sizeof(uint32_t); > + *(__packed uint64_t *)data = qid->path; > + data += sizeof(uint64_t); > + break; > + > + case 'S': > + str_val = par; > + elem_sz = sizeof(str_val); > + len = strlen(str_val); Should len be checked to ensure it doesn't exceed 0xffff? Regards, Jason
On 02.11.23 19:48, Jason Andryuk wrote: > On Wed, Nov 1, 2023 at 6:56 AM Juergen Gross <jgross@suse.com> wrote: >> >> Add support for generation a 9pfs protocol response via a format based >> approach. >> >> Strings are stored in a per device string buffer and they are >> referenced via their offset in this buffer. This allows to avoid >> having to dynamically allocate memory for each single string. >> >> As a first user of the response handling add a generic p9_error() >> function which will be used to return any error to the client. >> >> Add all format parsing variants in order to avoid additional code churn >> later when adding the users of those variants. Prepare a special case >> for the "read" case already (format character 'D'): in order to avoid >> adding another buffer for read data support doing the read I/O directly >> into the response buffer. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- > >> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c >> index 590d06e906..5a06f72338 100644 >> --- a/tools/xenlogd/io.c >> +++ b/tools/xenlogd/io.c > >> @@ -101,6 +112,172 @@ static bool io_work_pending(device *device) >> : ring_out_data(device); >> } >> >> +static void fmt_err(const char *fmt) >> +{ >> + syslog(LOG_CRIT, "illegal format %s passed to fill_buffer()", fmt); >> + exit(1); >> +} >> + >> +/* >> + * Fill buffer with response data. >> + * fmt is a sequence of format characters. Supported characters are: >> + * a: an array (2 bytes number of elements + the following format as elements) >> + * The number of elements is passed in the first unsigned int parameter, the >> + * next parameter is a pointer to an array of elements as denoted by the next >> + * format character. >> + * b: 2 byte unsigned integer >> + * The parameter is a pointer to a uint16_t value >> + * D: Data blob (4 byte length + <length> bytes) >> + * 2 parameters are consumed, first an unsigned int for the length, then a >> + * pointer to the first uint8_t value. >> + * No array support. >> + * L: 8 byte unsigned integer >> + * The parameter is a pointer to a uint64_t value >> + * Q: Qid (struct p9_qid) >> + * S: String (2 byte length + <length> characters) >> + * The length is obtained via strlen() of the parameter, being a pointer >> + * to the first character of the string >> + * U: 4 byte unsigned integer >> + * The parameter is a pointer to a uint32_t value >> + */ >> +static void fill_buffer(device *device, uint8_t cmd, uint16_t tag, >> + const char *fmt, ...) >> +{ >> + struct p9_header *hdr = device->buffer; >> + void *data = hdr + 1; >> + const char *f; >> + const void *par; >> + const char *str_val; >> + const struct p9_qid *qid; >> + unsigned int len; >> + va_list ap; >> + unsigned int array_sz = 0; >> + unsigned int elem_sz = 0; >> + >> + hdr->cmd = cmd; >> + hdr->tag = tag; >> + >> + va_start(ap, fmt); >> + >> + for ( f = fmt; *f; f++ ) >> + { >> + if ( !array_sz ) >> + par = va_arg(ap, const void *); >> + else >> + { >> + par += elem_sz; >> + array_sz--; >> + } >> + >> + switch ( *f ) >> + { >> + case 'a': >> + f++; >> + if ( !*f || array_sz ) >> + fmt_err(fmt); >> + array_sz = *(const unsigned int *)par; >> + *(__packed uint16_t *)data = array_sz; > > Is it worth checking that array_sz doesn't exceed 0xffff? I can add that. > >> + data += sizeof(uint16_t); >> + par = va_arg(ap, const void *); >> + elem_sz = 0; >> + break; >> + >> + case 'u': >> + *(__packed uint16_t *)data = *(const uint16_t *)par; >> + elem_sz = sizeof(uint16_t); >> + data += sizeof(uint16_t); >> + break; >> + >> + case 'D': >> + if ( array_sz ) >> + fmt_err(fmt); >> + len = *(const unsigned int *)par; >> + *(__packed uint32_t *)data = len; >> + data += sizeof(uint32_t); >> + par = va_arg(ap, const void *); >> + if ( data != par ) >> + memcpy(data, par, len); >> + data += len; >> + break; >> + >> + case 'L': >> + *(__packed uint64_t *)data = *(const uint64_t *)par; >> + elem_sz = sizeof(uint64_t); >> + data += sizeof(uint64_t); >> + break; >> + >> + case 'Q': >> + qid = par; >> + elem_sz = sizeof(*qid); >> + *(uint8_t *)data = qid->type; >> + data += sizeof(uint8_t); >> + *(__packed uint32_t *)data = qid->version; >> + data += sizeof(uint32_t); >> + *(__packed uint64_t *)data = qid->path; >> + data += sizeof(uint64_t); >> + break; >> + >> + case 'S': >> + str_val = par; >> + elem_sz = sizeof(str_val); >> + len = strlen(str_val); > > Should len be checked to ensure it doesn't exceed 0xffff? I'll add that check. Juergen
diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c index 590d06e906..5a06f72338 100644 --- a/tools/xenlogd/io.c +++ b/tools/xenlogd/io.c @@ -11,6 +11,7 @@ * before looking for the next request. */ +#include <errno.h> #include <stdbool.h> #include <stdlib.h> #include <string.h> @@ -20,6 +21,16 @@ #include "xenlogd.h" +/* P9 protocol commands (response is either cmd+1 or P9_CMD_ERROR). */ +#define P9_CMD_ERROR 107 + +struct p9_qid { + uint8_t type; +#define QID_TYPE_DIR 0x80 + uint32_t version; + uint64_t path; +}; + /* * Note that the ring names "in" and "out" are from the frontend's * perspective, so the "in" ring will be used for responses to the frontend, @@ -101,6 +112,172 @@ static bool io_work_pending(device *device) : ring_out_data(device); } +static void fmt_err(const char *fmt) +{ + syslog(LOG_CRIT, "illegal format %s passed to fill_buffer()", fmt); + exit(1); +} + +/* + * Fill buffer with response data. + * fmt is a sequence of format characters. Supported characters are: + * a: an array (2 bytes number of elements + the following format as elements) + * The number of elements is passed in the first unsigned int parameter, the + * next parameter is a pointer to an array of elements as denoted by the next + * format character. + * b: 2 byte unsigned integer + * The parameter is a pointer to a uint16_t value + * D: Data blob (4 byte length + <length> bytes) + * 2 parameters are consumed, first an unsigned int for the length, then a + * pointer to the first uint8_t value. + * No array support. + * L: 8 byte unsigned integer + * The parameter is a pointer to a uint64_t value + * Q: Qid (struct p9_qid) + * S: String (2 byte length + <length> characters) + * The length is obtained via strlen() of the parameter, being a pointer + * to the first character of the string + * U: 4 byte unsigned integer + * The parameter is a pointer to a uint32_t value + */ +static void fill_buffer(device *device, uint8_t cmd, uint16_t tag, + const char *fmt, ...) +{ + struct p9_header *hdr = device->buffer; + void *data = hdr + 1; + const char *f; + const void *par; + const char *str_val; + const struct p9_qid *qid; + unsigned int len; + va_list ap; + unsigned int array_sz = 0; + unsigned int elem_sz = 0; + + hdr->cmd = cmd; + hdr->tag = tag; + + va_start(ap, fmt); + + for ( f = fmt; *f; f++ ) + { + if ( !array_sz ) + par = va_arg(ap, const void *); + else + { + par += elem_sz; + array_sz--; + } + + switch ( *f ) + { + case 'a': + f++; + if ( !*f || array_sz ) + fmt_err(fmt); + array_sz = *(const unsigned int *)par; + *(__packed uint16_t *)data = array_sz; + data += sizeof(uint16_t); + par = va_arg(ap, const void *); + elem_sz = 0; + break; + + case 'u': + *(__packed uint16_t *)data = *(const uint16_t *)par; + elem_sz = sizeof(uint16_t); + data += sizeof(uint16_t); + break; + + case 'D': + if ( array_sz ) + fmt_err(fmt); + len = *(const unsigned int *)par; + *(__packed uint32_t *)data = len; + data += sizeof(uint32_t); + par = va_arg(ap, const void *); + if ( data != par ) + memcpy(data, par, len); + data += len; + break; + + case 'L': + *(__packed uint64_t *)data = *(const uint64_t *)par; + elem_sz = sizeof(uint64_t); + data += sizeof(uint64_t); + break; + + case 'Q': + qid = par; + elem_sz = sizeof(*qid); + *(uint8_t *)data = qid->type; + data += sizeof(uint8_t); + *(__packed uint32_t *)data = qid->version; + data += sizeof(uint32_t); + *(__packed uint64_t *)data = qid->path; + data += sizeof(uint64_t); + break; + + case 'S': + str_val = par; + elem_sz = sizeof(str_val); + len = strlen(str_val); + *(__packed uint16_t *)data = len; + data += sizeof(uint16_t); + memcpy(data, str_val, len); + data += len; + break; + + case 'U': + *(__packed uint32_t *)data = *(const uint32_t *)par; + elem_sz = sizeof(uint32_t); + data += sizeof(uint32_t); + break; + + default: + fmt_err(fmt); + } + + if ( array_sz ) + f--; + } + + hdr->size = data - device->buffer; +} + +static unsigned int add_string(device *device, const char *str, + unsigned int len) +{ + char *tmp; + unsigned int ret; + + if ( device->str_used + len + 1 > device->str_size ) + { + tmp = realloc(device->str, device->str_used + len + 1); + if ( !tmp ) + return ~0; + device->str = tmp; + device->str_size = device->str_used + len + 1; + } + + ret = device->str_used; + memcpy(device->str + ret, str, len); + device->str_used += len; + device->str[device->str_used++] = 0; + + return ret; +} + +static void p9_error(device *device, uint16_t tag, uint32_t err) +{ + unsigned int erroff; + + strerror_r(err, device->buffer, device->ring_size); + erroff = add_string(device, device->buffer, strlen(device->buffer)); + fill_buffer(device, P9_CMD_ERROR, tag, "SU", + erroff != ~0 ? device->str + erroff : "cannot allocate memory", + &err); +} + void *io_thread(void *arg) { device *device = arg; @@ -152,7 +329,16 @@ void *io_thread(void *arg) if ( count < hdr.size ) continue; - /* TODO: handle request. */ + device->str_used = 0; + + switch ( hdr.cmd ) + { + default: + syslog(LOG_DEBUG, "%u.%u sent unhandled command %u\n", + device->domid, device->devid, hdr.cmd); + p9_error(device, hdr.tag, EOPNOTSUPP); + break; + } device->handle_response = true; hdr.size = ((struct p9_header *)device->buffer)->size; @@ -175,6 +361,7 @@ void *io_thread(void *arg) } } + free(device->str); free(device->buffer); device->thread_active = false; diff --git a/tools/xenlogd/xenlogd.h b/tools/xenlogd/xenlogd.h index 40aa7cd03a..c10c6aa9e5 100644 --- a/tools/xenlogd/xenlogd.h +++ b/tools/xenlogd/xenlogd.h @@ -57,6 +57,9 @@ struct device { bool error; /* Protocol error - stop processing. */ bool handle_response; /* Main loop now handling response. */ void *buffer; /* Request/response buffer. */ + char *str; /* String work space. */ + unsigned int str_size; /* Size of *str. */ + unsigned int str_used; /* Currently used size of *str. */ }; extern xenevtchn_handle *xe;
Add support for generation a 9pfs protocol response via a format based approach. Strings are stored in a per device string buffer and they are referenced via their offset in this buffer. This allows to avoid having to dynamically allocate memory for each single string. As a first user of the response handling add a generic p9_error() function which will be used to return any error to the client. Add all format parsing variants in order to avoid additional code churn later when adding the users of those variants. Prepare a special case for the "read" case already (format character 'D'): in order to avoid adding another buffer for read data support doing the read I/O directly into the response buffer. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenlogd/io.c | 189 +++++++++++++++++++++++++++++++++++++++- tools/xenlogd/xenlogd.h | 3 + 2 files changed, 191 insertions(+), 1 deletion(-)