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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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.

Having written that, I don't see any issue with the code as-is, so if
you don't want to make any further change:
Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

Regards,
Jason