Message-Id: <201012061355.44032.arnd@arndb.de>
Date:	Mon, 6 Dec 2010 13:55:43 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Charles Manning <manningc2@...rix.gen.nz>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Steven Rostedt <rostedt@...dmis.org>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH 3/8] Add yaffs2 file system: guts code

On Monday 06 December 2010, Charles Manning wrote:
> On Wednesday 01 December 2010 11:23:53 you wrote:
> > On Tuesday 30 November 2010 22:57:29 Charles Manning wrote:
> > It would be better to reorder the functions in each file so that
> > you don't need forward declarations. This generally makes reading
> > the code easier because it is what people expect to see. It
> > also makes it clearer where you have possible recursions in the code.
> 
> Hmmm.. 
> I too prefer minimal use of forward declaration.
> Some of them are because I copied the layout of existing kernel code which 
> uses fwd declarations a lot. eg. fs/jffs2/dir.c  and many of the examples in 
> Rubini & Corbet.

There is not much point in changing the legacy code that's already in
the kernel, but let's try to keep it clean for new code. We have a lot
of bad examples for coding style that we wouldn't merge these days.

In this case, it should be an obvious change with no real downsides.

> > > +	T(YAFFS_TRACE_BUFFERS,
> > > +	  (TSTR("Out of temp buffers at line %d, other held by lines:"),
> > > +	   line_no));
> > > +	for (i = 0; i < YAFFS_N_TEMP_BUFFERS; i++)
> > > +		T(YAFFS_TRACE_BUFFERS,
> > > +		  (TSTR(" %d "), dev->temp_buffer[i].line));
> > > +
> > > +	T(YAFFS_TRACE_BUFFERS, (TSTR(" " TENDSTR)));
> 
> > The tracing functions are rather obscure. I would recommend dropping
> > them all for now, in order to get the code included.
> 
> Yup that was a very ugly bunch of hackery to make WinCE unicode work.
> 
> I think I can pull the var-arg-foo to replace these with
> 
> yaffs_trace(YAFFS_TRACE_BUFFERS,
> 	"Out of temp buffers at line %d, other held by lines:",line_no);
> for (i = 0; i < YAFFS_N_TEMP_BUFFERS; i++)
> 	yaffs_trace(YAFFS_TRACE_BUFFERS," %d ", dev->temp_buffer[i].line);
> yaffs_trace(YAFFS_TRACE_BUFFERS, "\n");
> 
> Would that be OK?
> 
> I am loath to have to pull out useful code then plug it back in again.

I don't think the yaffs_trace() function would be much better than the T()
macro, I was talking more about the fact that you have your own nonstandard
tracing infrastructure than the ugliness of the interface.

The point of pulling it out now would be force you to rethink the
tracing. If you think that you'd arrive at the same conclusion, just
save the diff between the code with and without tracing so you can
submit that patch again later.

Having some sort of tracing is clearly useful, but it's also not essential
for the basic yaffs2 operation. We want to keep a consistent way of
presenting trace points across the kernel, so as long as you do it
differently, your code is going to be viewed with some suspicion.

Please have a look at how ext4, gfs2 and xfs do tracing.

> > At a later 
> > stage, you can add standard trace points.
> >
> > > +	return YMALLOC(dev->data_bytes_per_chunk);
> 
> I'm getting rid of those..
> 
> The reason for thew wrapping was to make portable code.
> 
> I'm replacing these with kmalloc() and then providing kmalloc() which is just 
> a wrapped malloc() for non-Linux use.

Ok, excellent!
 
> I am hoping to reduce this to YCHAR, _Y() and maybe one or two others.

Ok. Maybe rename YCHAR to ychar to make it stick out less, and add a comment
to the definition why you need it then.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists