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

[Juergen Gross <jgross@xxxxxxxx>]

On 06.02.23 11:05, Samuel Thibault wrote:
> 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 ~ ?
Oh, indeed.

> > @@ -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.
Yes, might be better.

> > +    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"?
I'd rather add a check for the file name to be properly canonicalized
further up in the hierarchy (either in open() directly, or in open_9pfs()).

> >   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)?
close() does that via close_9pfs().


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature