Re: [PATCH 05/29] tools/xenlogd: add 9pfs response generation support

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Nov 1, 2023 at 6:56 AM Juergen Gross <jgross@xxxxxxxx> 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@xxxxxxxx>
> ---

> 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