Re: [PATCH 6/7] Mini-OS: add open and close handling to the 9pfs frontend

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le ven. 03 févr. 2023 10:18:08 +0100, a ecrit:
> +static unsigned int get_fid(struct dev_9pfs *dev)
> +{
> +    unsigned int fid;
> +
> +    fid = ffs(dev->fid_mask);
> +    if ( fid )
> +        dev->fid_mask &= 1ULL << (fid - 1);

Isn't that missing a ~ ?

> @@ -459,6 +522,134 @@ static int p9_attach(struct dev_9pfs *dev)
>      return ret;
>  }
>  
> +static int p9_clunk(struct dev_9pfs *dev, uint32_t fid)
> +{
> +    struct req *req = get_free_req(dev);
> +    int ret;
> +
> +    if ( !req )
> +        return ENOENT;

Rather EAGAIN? (and similar in other functions)
(actually p9_version and p9_attach could be updated too, even if in
their case it's not supposed to happen.

> +    req->cmd = P9_CMD_CLUNK;
> +    send_9p(dev, req, "U", fid);
> +    rcv_9p(dev, req, "");
> +    ret = req->result;
> +
> +    put_free_req(dev, req);
> +
> +    return ret;
> +}


> +static unsigned int split_path(const char *pathname, char **split_ptr)
> +{
> +    unsigned int parts = 1;
> +    char *p;
> +
> +    *split_ptr = strdup(pathname);
> +
> +    for ( p = strchr(*split_ptr, '/'); p; p = strchr(p + 1, '/') )
> +    {
> +        *p = 0;
> +        parts++;
> +    }
> +
> +    return parts;
> +}

Do we need to care about an empty part if we're passed "/foo//bar"?

>  static int open_9pfs(struct mount_point *mnt, const char *pathname, int 
> flags,
>                       mode_t mode)
>  {
> -    errno = ENOSYS;
> +    int fd;
> +    char *path = NULL;
> +    char *p;
> +    struct file *file;
> +    struct file_9pfs *f9pfs;
> +    uint16_t nwalk;
> +    uint8_t omode;
> +    int ret;
> +
> +    f9pfs = calloc(1, sizeof(*f9pfs));
> +    f9pfs->dev = mnt->dev;
> +    f9pfs->fid = P9_ROOT_FID;
> +
> +    fd = alloc_fd(ftype_9pfs);
> +    file = get_file_from_fd(fd);
> +    file->filedata = f9pfs;
> +
> +    switch ( flags & O_ACCMODE )
> +    {
> +    case O_RDONLY:
> +        omode = P9_OREAD;
> +        break;
> +    case O_WRONLY:
> +        omode = P9_OWRITE;
> +        break;
> +    case O_RDWR:
> +        omode = P9_ORDWR;
> +        break;
> +    default:
> +        ret = EINVAL;
> +        goto err;
> +    }
[...]
> + err:
> +    free(path);
> +    close(fd);
> +    errno = ret;

This seems missing free(f9pfs)?

Samuel