Re: [PATCH 3/7] v9fs: VFS inode operations (2.0)

From: Alexey Dobriyan
Date: Wed Jun 08 2005 - 11:57:21 EST

Next message: Dan A. Dickey: "Re: System state too high for too long..."
Previous message: Karim Yaghmour: "Re: Attempted summary of "RT patch acceptance" thread"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tuesday 07 June 2005 18:49, ericvh@xxxxxxxxx wrote:
> This is part [3/7] of the v9fs-2.0 patch against Linux 2.6.
>
> This part of the patch contains the VFS inode interfaces.

More findings at the micro level.

> --- /dev/null
> +++ b/fs/9p/vfs_inode.c

> + * This file contians vfs inode ops for the 9P2000 protocol.

contains

> +static inline int unixmode2p9mode(struct v9fs_session_info *v9ses, int mode)
> +{
> + int res;
> + res = mode & 0777;
> + if (S_ISDIR(mode))
> + res |= V9FS_DMDIR;
> + if (v9ses->extended) {
> + if (S_ISLNK(mode))
> + res |= V9FS_DMSYMLINK;
> + if (v9ses->nodev == 0) {
> + if (S_ISSOCK(mode))
> + res |= V9FS_DMSOCKET;
> + if (S_ISFIFO(mode))
> + res |= V9FS_DMNAMEDPIPE;
> + if (S_ISBLK(mode))
> + res |= V9FS_DMDEVICE;
> + if (S_ISCHR(mode))
> + res |= V9FS_DMDEVICE;
> + }
> +
> + if ((mode & S_ISUID) == S_ISUID)
> + res |= V9FS_DMSETUID;
> + if ((mode & S_ISGID) == S_ISGID)
> + res |= V9FS_DMSETGID;
> + if ((mode & V9FS_DMLINK))
> + res |= V9FS_DMLINK;
> + }
> +
> + return res;
> +}

> +static inline int p9mode2unixmode(struct v9fs_session_info *v9ses, int mode)
> +{
> + int res;
> +
> + res = mode & 0777;
> +
> + if ((mode & V9FS_DMDIR) == V9FS_DMDIR)
> + res |= S_IFDIR;
> + else if ((mode & V9FS_DMSYMLINK) && (v9ses->extended))
> + res |= S_IFLNK;
> + else if ((mode & V9FS_DMSOCKET) && (v9ses->extended)
> + && (v9ses->nodev == 0))
> + res |= S_IFSOCK;
> + else if ((mode & V9FS_DMNAMEDPIPE) && (v9ses->extended)
> + && (v9ses->nodev == 0))
> + res |= S_IFIFO;
> + else if ((mode & V9FS_DMDEVICE) && (v9ses->extended)
> + && (v9ses->nodev == 0))
> + res |= S_IFBLK;
> + else
> + res |= S_IFREG;
> +
> + if (v9ses->extended) {
> + if ((mode & V9FS_DMSETUID) == V9FS_DMSETUID)
> + res |= S_ISUID;
> +
> + if ((mode & V9FS_DMSETGID) == V9FS_DMSETGID)
> + res |= S_ISGID;
> + }
> +
> + return res;
> +}

> +static inline void
> +v9fs_blank_mistat(struct v9fs_session_info *v9ses, struct v9fs_stat *mistat)
> +{
> + mistat->type = ~0;
> + mistat->dev = ~0;
> + mistat->qid.type = ~0;
> + mistat->qid.version = ~0;
> + *((long long *)&mistat->qid.path) = ~0;
> + mistat->mode = ~0;
> + mistat->atime = ~0;
> + mistat->mtime = ~0;
> + mistat->length = ~0;
> + mistat->name = mistat->data;
> + mistat->uid = mistat->data;
> + mistat->gid = mistat->data;
> + mistat->muid = mistat->data;
> + if (v9ses->extended) {
> + mistat->n_uid = ~0;
> + mistat->n_gid = ~0;
> + mistat->n_muid = ~0;
> + mistat->extension = mistat->data;
> + }
> + *mistat->data = 0;
> +}

Approximately 178, 150 and 122 bytes of object code, respectively. Do you
really want them to be inlined?

> +/**
> + * v9fs_mistat2unix - convert mistat to unix stat
> + * @mistat: Plan 9 metadata (mistat) structure
> + * @stat: unix metadata (stat) structure to populate
> + * @sb: superblock
> + *
> + */
> +
> +static void
> +v9fs_mistat2unix(struct v9fs_stat *mistat, struct stat *buf,
> + struct super_block *sb)

Comments and arguments don't match.

> +{

> + if (v9ses && v9ses->extended) {
> + /* TODO: string to uid mapping via user-space daemon */
> + buf->st_uid = mistat->n_uid;
> + buf->st_gid = mistat->n_gid;
> +
> + sscanf(mistat->uid, "%x", (unsigned int *)&buf->st_uid);
> + sscanf(mistat->gid, "%x", (unsigned int *)&buf->st_gid);
> + } else {
> + buf->st_uid = v9ses->uid;
> + buf->st_gid = v9ses->gid;
> + }

??? st_uid and st_gid will be overwritten.

> + buf->st_uid = (unsigned short)-1;
> + buf->st_gid = (unsigned short)-1;
> +
> + if (v9ses && v9ses->extended) {
> + if (mistat->n_uid != -1)
> + sscanf(mistat->uid, "%x", (unsigned int *)&buf->st_uid);
> +
> + if (mistat->n_gid != -1)
> + sscanf(mistat->gid, "%x", (unsigned int *)&buf->st_gid);
> + }
> +
> + if (buf->st_uid == (unsigned short)-1)
> + buf->st_uid = v9ses->uid;
> + if (buf->st_gid == (unsigned short)-1)
> + buf->st_gid = v9ses->gid;

> +/**
> + * v9fs_create - helper function to create files and directories

> + * @open_mode: resulting open mode for file ???

You're not sure?

> +/**
> + * v9fs_remove - helper function to remove files and directories
> + * @inode: directory inode that is being deleted
> + * @dentry: dentry that is being deleted
> + * @rmdir: where we are a file or a directory
> + *
> + */
> +
> +static int v9fs_remove(struct inode *dir, struct dentry *file, int rmdir)

Comment and arguments don't match.

> +/**
> + * v9fs_vfs_mkdir - VFS mkdir hook to create a directory
> + * @i: inode that is being unlinked

@inode

> + * @dentry: dentry that is being unlinked
> + * @mode: mode for new directory

> +static int v9fs_vfs_mkdir(struct inode *inode, struct dentry *dentry, int mode)

> +/**
> + * v9fs_vfs_unlink - VFS unlink hook to delete an inode
> + * @i: inode that is being unlinked
> + * @dentry: dentry that is being unlinked
> + *
> + */
> +
> +static int v9fs_vfs_unlink(struct inode *i, struct dentry *d)

Comment and arguments don't match.

> +/**
> + * v9fs_vfs_rmdir - VFS unlink hook to delete a directory
> + * @i: inode that is being unlinked
> + * @dentry: dentry that is being unlinked
> + *
> + */
> +
> +static int v9fs_vfs_rmdir(struct inode *i, struct dentry *d)

Comment and arguments don't match.

> +static int
> +v9fs_vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> + struct inode *new_dir, struct dentry *new_dentry)
> +{

> + struct v9fs_stat *mistat = kmalloc(v9ses->maxdata, GFP_KERNEL);

Unchecked kmalloc.

> +static int v9fs_vfs_setattr(struct dentry *dentry, struct iattr *iattr)
> +{

> + struct v9fs_stat *mistat = kmalloc(v9ses->maxdata, GFP_KERNEL);

Better move right before !mistat check.

> + struct v9fs_fcall *fcall = NULL;
> + int res = -EPERM;
> +
> + dprintk(DEBUG_VFS, "\n");
> + if (!fid) {
> + dprintk(DEBUG_ERROR,
> + "Couldn't find fid associated with dentry\n");
> + return -EBADF;
> + }
> +
> + if (!mistat)
> + return -ENOMEM;

> + if (v9ses->extended) {
> + char *uid = kmalloc(strlen(mistat->uid), GFP_KERNEL);
> + char *gid = kmalloc(strlen(mistat->gid), GFP_KERNEL);
> + char *muid = kmalloc(strlen(mistat->muid), GFP_KERNEL);
> + char *name = kmalloc(strlen(mistat->name), GFP_KERNEL);
> + char *extension = kmalloc(strlen(mistat->extension),
> + GFP_KERNEL);
> +
> + if ((!uid) || (!gid) || (!muid) || (!name) || (!extension)) {
> + kfree(uid);
> + kfree(gid);
> + kfree(muid);
> + kfree(name);
> + kfree(extension);
> +
> + return -ENOMEM;

kfree(mistat);

> + if (iattr->ia_valid & ATTR_UID) {
> + if (strlen(uid) != 8) {
> + dprintk(DEBUG_ERROR, "uid strlen is %u not 8\n",
> + (unsigned int)strlen(uid));
> + sprintf(uid, "%08x", iattr->ia_uid);
> + } else {
> + kfree(uid);
> + uid = kmalloc(9, GFP_KERNEL);

Unchecked kmalloc.

> + }
> +
> + sprintf(uid, "%08x", iattr->ia_uid);

Double sprintf.

> + mistat->n_uid = iattr->ia_uid;
> + }
> +
> + if (iattr->ia_valid & ATTR_GID) {
> + if (strlen(gid) != 8)
> + dprintk(DEBUG_ERROR, "gid strlen is %u not 8\n",
> + (unsigned int)strlen(gid));
> + else {
> + kfree(gid);
> + gid = kmalloc(9, GFP_KERNEL);
> + }
> +
> + sprintf(gid, "%08x", iattr->ia_gid);
> + mistat->n_gid = iattr->ia_gid;
> + }

See uid.

> +static int
> +v9fs_vfs_symlink(struct inode *dir, struct dentry *dentry, const char *symname)
> +{
> + struct v9fs_stat *mistat = kmalloc(v9ses->maxdata, GFP_KERNEL);

Unchecked kmalloc.

> + v9fs_blank_mistat(v9ses, mistat);

> +/**
> + * v9fs_readlink - read a symlink's location (internal version)

> + * @buf: buffer to load symlink location into

@buffer

> +static int v9fs_readlink(struct dentry *dentry, char *buffer, int buflen)

> +/**
> + * v9fs_vfs_link - create a hardlink

> + * @new_dentry: dentry for link

@dentry. Or new_dentry in the code.

> +static int
> +v9fs_vfs_link(struct dentry *old_dentry, struct inode *dir,
> + struct dentry *dentry)
> +{

> + struct v9fs_stat *mistat = kmalloc(v9ses->maxdata, GFP_KERNEL);

Unchecked kmalloc.

> +/**
> + * v9fs_vfs_mknod - create a special file

> + * @dev_t: device associated with special file

@rdev

> +static int
> +v9fs_vfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
> +{

> + struct v9fs_stat *mistat = kmalloc(v9ses->maxdata, GFP_KERNEL);

Guess what? :-)

> + else if (S_ISFIFO(mode)) ; /* DO NOTHING */
> + else {

else if (S_ISFIFO(mode))
;
else {

means "do nothing" perfectly.

> + if (!S_ISFIFO(mode)) {
> + /* issue a twstat */
> + v9fs_blank_mistat(v9ses, mistat);
> + strcpy(mistat->data + 1, symname);
> + mistat->extension = mistat->data + 1;
> + retval = v9fs_t_wstat(v9ses, newfid->fid, mistat, &fcall);
> + if (retval < 0) {
> + dprintk(DEBUG_ERROR, "v9fs_t_wstat error: %s\n",
> + FCALL_ERROR(fcall));
> + goto FreeMem;
> + }
> +
> + kfree(fcall);

Will be freed in a line.

> + }
> +
> + /* need to update dcache so we show up */
> + kfree(fcall);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Dan A. Dickey: "Re: System state too high for too long..."
Previous message: Karim Yaghmour: "Re: Attempted summary of "RT patch acceptance" thread"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]