Messages in this thread | | | Date | Sun, 22 Jun 2008 23:51:22 +0300 | From | Eduard - Gabriel Munteanu <> | Subject | Re: [PATCH 3/3] relay: Add buffer-only channels; useful for early logging. |
| |
On Sun, 22 Jun 2008 22:29:53 +0300 "Pekka Enberg" <penberg@cs.helsinki.fi> wrote:
> On Sat, Jun 21, 2008 at 5:11 PM, Eduard - Gabriel Munteanu > <eduard.munteanu@linux360.ro> wrote: > > + tmpname = kzalloc(NAME_MAX + 1, GFP_KERNEL); > > + if (!tmpname) > > + goto failed; > > Why don't you return -ENOMEM here?
The original function from which I ripped this thing off (relay_open_buf()) returned 1 on error, so I assumed I wasn't supposed to change this behavior.
I should probably do this in a separate patch.
> The separate goto seems pointless. Why don't you use negative return > values for error handling?
Yes, it's better to use 'return' directly.
> > /** > > + * relay_late_setup_files - triggers file creation > > + * @chan: channel to operate on > > + * @base_filename: base name of files to create > > + * @parent: dentry of parent directory, %NULL for root > > directory > > + * > > + * Returns 0 if successful, non-zero otherwise. > > + * > > + * Use to setup files for a previously buffer-only channel. > > + * Useful to do early tracing in kernel, before VFS is up, for > > example. > > + */ > > +int relay_late_setup_files(struct rchan *chan, > > + const char *base_filename, > > + struct dentry *parent) > > +{ > > + unsigned int i; > > + int err; > > + struct rchan_buf *buf; > > + > > + if (!chan || !base_filename) > > + return 1; > > -EINVAL?
Will do and resubmit. Being new code (called directly by other kernel users), it allows me to be a bit inconsistent about return values.
> > + err = relay_setup_buf_file(chan, buf, i); > > + if (unlikely(err)) > > + goto out; > > + } > > + mutex_unlock(&relay_channels_mutex); > > + > > + return 0; > > + > > +out: > > + mutex_unlock(&relay_channels_mutex); > > + return 1; > > You don't need two return statements (and coincidentally two unlocks) > if you keep the return value in a local variable 'err'.
Thanks, I missed this possibility. Sounds better.
Eduard
| |