Re: [PATCH 07/29] tools/xenlogd: add 9pfs attach request support

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Nov 1, 2023 at 5:54 AM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> Add the attach request of the 9pfs protocol. This introduces the "fid"
> scheme of the 9pfs protocol.
>
> As this will be needed later, use a dedicated memory allocation
> function in alloc_fid().
>
> For filling the qid data take the approach from the qemu 9pfs backend
> implementation.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  tools/xenlogd/io.c      | 128 ++++++++++++++++++++++++++++++++++++++++
>  tools/xenlogd/xenlogd.c |   1 +
>  tools/xenlogd/xenlogd.h |  11 ++++
>  3 files changed, 140 insertions(+)
>
> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index f35520018f..fa825c9f39 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c

> +static struct p9_fid *alloc_fid_mem(device *device, unsigned int fid,
> +                                    const char *path)
> +{
> +    struct p9_fid *fidp;
> +    size_t pathlen;
> +
> +    pathlen = strlen(device->host_path) + strlen(path) + 1;
> +    fidp = calloc(sizeof(*fidp) + pathlen, 1);
> +    if ( !fidp )
> +        return NULL;
> +
> +    fidp->fid = fid;
> +    snprintf(fidp->path, pathlen, "%s%s", device->host_path, path);

check_host_path() should be enhanced to ensure host_path has a
trailing '/', or switch this to "%s/%s" to ensure it's always present?

> +
> +    return fidp;
> +}
> +


> +static void free_fid(device *device, struct p9_fid *fidp)
> +{
> +    if ( !fidp )
> +        return;
> +
> +    device->n_fids--;
> +    XEN_TAILQ_REMOVE(&device->fids, fidp, list);
> +    free(fidp);
> +}
> +
> +static int fill_qid(const char *path, struct p9_qid *qid, struct stat *stbuf)

Nit: ordering is input, output, optional input, so you might want to re-order?

stbuf can be const?

> +{
> +    struct stat st;
> +
> +    if ( !stbuf )
> +    {
> +        if ( stat(path, &st) )
> +            return errno;
> +
> +        stbuf = &st;
> +    }
> +
> +    qid->type = S_ISDIR(stbuf->st_mode) ? QID_TYPE_DIR : 0;
> +    qid->version = stbuf->st_mtime ^ (stbuf->st_size << 8);
> +    qid->path = stbuf->st_ino;
> +
> +    return 0;
> +}
> +
>  static void p9_error(device *device, uint16_t tag, uint32_t err)
>  {
>      unsigned int erroff;
> @@ -476,6 +565,41 @@ static void p9_version(device *device, struct p9_header 
> *hdr)
>                  version);
>  }
>
> +static void p9_attach(device *device, struct p9_header *hdr)
> +{
> +    uint32_t fid;
> +    uint32_t dummy_u32;
> +    unsigned int dummy_uint;
> +    struct p9_qid qid;
> +    int ret;
> +
> +    ret = fill_data(device, "UUSSU", &fid, &dummy_u32, &dummy_uint, 
> &dummy_uint,
> +                    &dummy_u32);
> +    if ( ret != 5 )
> +    {
> +        p9_error(device, hdr->tag, errno);
> +        return;
> +    }

We might want to check the first dummy_u32 (afid) to ensure it's NOFID?
"""
If the client does not wish to authenticate the connection, or knows
that authentication is not required, the afid field in the attach mes-
sage should be set to NOFID, defined as (u32int)~0 in fcall.h. If the
client does wish to authenticate, it must acquire and validate an afid
using an auth message before doing the attach.
"""

Since auth isn't implemented, it's probably not necessary to check afid?

I've been looking at these as reference:
https://ericvh.github.io/9p-rfc/rfc9p2000.html
https://ericvh.github.io/9p-rfc/rfc9p2000.u.html

> +
> +    device->root_fid = alloc_fid(device, fid, "");
> +    if ( !device->root_fid )
> +    {
> +        p9_error(device, hdr->tag, errno);
> +        return;
> +    }
> +
> +    ret = fill_qid(device->host_path, &qid, NULL);
> +    if ( ret )
> +    {
> +        free_fid(device, device->root_fid);

root_fid is only freed in this error path.  Maybe free_device() should
free all the fids?

Regards,
Jason