Re: [PATCH v3 08/33] tools/xenlogd: add 9pfs attach request support

[Jürgen Groß <jgross@xxxxxxxx>]

On 09.01.24 19:48, Jason Andryuk wrote:
> On Thu, Jan 4, 2024 at 4:12 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() and prepare a fid reference count.
> >
> > For filling the qid data take the approach from the qemu 9pfs backend
> > implementation.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - make fill_qid() parameter stbuf const (Jason Andryuk)
> > - free fids after disconnecting guest (Jason Andryuk)
> > V3:
> > - only store relative path in fid (Jason Andryuk)
> The code looks good.  I did have a thought though.
>
> > +static struct p9_fid *alloc_fid_mem(device *device, unsigned int fid,
> > +                                    const char *path)
> > +{
> > +    struct p9_fid *fidp;
> > +    size_t pathlen;
> > +
> > +    /* Paths always start with "/" as they are starting at the mount point. */
> > +    assert(path[0] == '/');
> > +
> ...
>
> > +
> > +static const char *relpath_from_path(const char *path)
> > +{
> > +    if (!strcmp(path, "/"))
> > +        return ".";
> > +
> > +    return (path[0] == '/') ? path + 1 : path;
> > +}
> You've carefully written the code to ensure the *at() functions are
> not called with paths starting with "/".  What do you think about
> storing the converted paths when storing into the p9_fid?  That way
> the code doesn't have to worry about always going through
> relpath_from_path() before use.  Another option beside performing the
> relpath_from_path() conversion, would be to save fidp->path with "./"
> at the start to eliminate absolute paths that way.  My thinking is
> it's more robust to not have any absolute paths that could be passed
> to a *at() function.
I've had a thorough look at the code at the end of the series and I agree
this is a good idea.

I'll change the related patches accordingly.


Juergen