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

[Juergen Gross <jgross@xxxxxxxx>]

On 03.11.23 16:13, Jason Andryuk wrote:
> 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?
No, "path" is always starting with a "/" if it is not empty.

> > +
> > +    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?
Hmm, I did it on purpose, as the last parameter is optional (as you said).

> stbuf can be const?
Yes.

> > +{
> > +    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?
That was the idea, yes.

> 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?
No, but io_thread() should do so at the end.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature