Re: relayfs redux, part 5

[Pekka J Enberg]

Hi Tom, 

More nitpick follows. 

> diff -urpN -X dontdiff linux-2.6.10/fs/Makefile linux-2.6.10-cur/fs/Makefile
> --- linux-2.6.10/fs/Makefile	2004-12-24 15:34:58.000000000 -0600
> +++ linux-2.6.10-cur/fs/Makefile	2005-02-06 21:32:49.000000000 -0600
> +/**
> + *	relay_create_buf - allocate and initialize a channel buffer
> + *	@alloc_size: size of the buffer to allocate
> + *	@n_subbufs: number of sub-buffers in the channel
> + *
> + *	Returns channel buffer if successful, NULL otherwise
> + */
> +struct rchan_buf *relay_create_buf(struct rchan *chan)
> +{
> +	struct rchan_buf *buf = kcalloc(1, sizeof(struct rchan_buf), GFP_KERNEL);
> +	if (!buf)
> +		return NULL;
> +
> +	buf->padding = kmalloc(chan->n_subbufs * sizeof(unsigned *), GFP_KERNEL);
> +	if (!buf->padding)
> +		goto free_buf;
> +	
> +	buf->commit = kmalloc(chan->n_subbufs * sizeof(unsigned *), GFP_KERNEL);
> +	if (!buf->commit)
> +		goto free_padding;
> +
> +	buf->start = relay_alloc_buf(chan->alloc_size, &buf->page_array, &buf->page_count);
> +	if (!buf->start)
> +		goto free_commit;
> +
> +	buf->chan = chan;
> +	kref_get(&buf->chan->kref);
> +	return buf;
> +
> +free_commit:
> +	kfree(buf->commit);
> +
> +free_padding:
> +	kfree(buf->padding);
> +
> +free_buf:
> +	kfree(buf);
> +	return NULL;
> +}

kfree() deals with NULL pointers and since we're zeroing all of them when
we call kcalloc(), you only need one failure goto where you unconditionally
execute all kfree() calls. 

> diff -urpN -X dontdiff linux-2.6.10/fs/relayfs/inode.c linux-2.6.10-cur/fs/relayfs/inode.c
> --- linux-2.6.10/fs/relayfs/inode.c	1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.10-cur/fs/relayfs/inode.c	2005-02-14 20:10:33.000000000 -0600
> @@ -0,0 +1,426 @@
> +/**
> + *	relayfs_create_file - create a file in the relay filesystem
> + *	@name: the name of the file to create
> + *	@parent: parent directory
> + *	@mode: mode, if not specied the default perms are used
> + *	@chan: channel associated with the file
> + *
> + *	Returns file dentry if successful, NULL otherwise.
> + *
> + *	The file will be created user r on behalf of current user.
> + */
> +struct dentry *relayfs_create_file(const char *name, struct dentry *parent,
> +				   int mode, struct rchan *chan)
> +{
> +	if (!mode)
> +		mode = S_IRUSR;
> +	mode = (mode & S_IALLUGO) | S_IFREG;
> +
> +	return relayfs_create_entry(name, parent, mode, chan);

Shouldn't we check if name is NULL here like in relayfs_create_dir? Perhaps
the check should be moved to relayfs_create_entry(). 

> diff -urpN -X dontdiff linux-2.6.10/include/linux/relayfs_fs.h linux-2.6.10-cur/include/linux/relayfs_fs.h
> --- linux-2.6.10/include/linux/relayfs_fs.h	1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.10-cur/include/linux/relayfs_fs.h	2005-02-14 01:32:16.000000000 -0600
> @@ -0,0 +1,269 @@
> +
> +/*
> + * Relay attribute flags
> + */
> +#define RELAY_MODE_CONTINUOUS		0x1
> +#define RELAY_MODE_NO_OVERWRITE		0x2

These are mutually exclusive so they're really a boolean that states
whether overwrite is allowed or not. Therefore please either make struct
rchan flags a boolean (it is only used for this) or turn the above to a 
single bit flag (cleared for not allowed, set for allowed) and then drop 
check_attribute_flags(). 

                     Pekka 
-
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/