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

[Juergen Gross <jgross@xxxxxxxx>]

On 02.11.23 19:48, Jason Andryuk wrote:
> 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?
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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature