From: "Pekka J Enberg" <penberg@cs.helsinki.fi>
To: Tom Zanussi <zanussi@us.ibm.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
Greg KH <greg@kroah.com>, Andrew Morton <akpm@osdl.org>,
Andi Kleen <ak@muc.de>, Roman Zippel <zippel@linux-m68k.org>,
Robert Wisniewski <bob@watson.ibm.com>,
Tim Bird <tim.bird@am.sony.com>,
Christoph Hellwig <hch@infradead.org>,
karim@opersys.com
Subject: Re: relayfs redux, part 5
Date: Mon, 21 Feb 2005 10:24:58 +0200 [thread overview]
Message-ID: <courier.42199ADA.000008F4@courier.cs.helsinki.fi> (raw)
In-Reply-To: <16918.10967.325166.756203@tut.ibm.com>
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
prev parent reply other threads:[~2005-02-21 8:25 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-02-18 17:50 [PATCH] relayfs redux, part 5 Tom Zanussi
2005-02-21 8:24 ` Pekka J Enberg [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=courier.42199ADA.000008F4@courier.cs.helsinki.fi \
--to=penberg@cs.helsinki.fi \
--cc=ak@muc.de \
--cc=akpm@osdl.org \
--cc=bob@watson.ibm.com \
--cc=greg@kroah.com \
--cc=hch@infradead.org \
--cc=karim@opersys.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tim.bird@am.sony.com \
--cc=zanussi@us.ibm.com \
--cc=zippel@linux-m68k.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).