linux-kernel - Re: [PATCH 3/8] Add yaffs2 file system: guts code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201012061450.41675.manningc2@actrix.gen.nz>
Date:	Mon, 6 Dec 2010 14:50:41 +1300
From:	Charles Manning <manningc2@...rix.gen.nz>
To:	Arnd Bergmann <arnd@...db.de>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/8] Add yaffs2 file system: guts code

On Wednesday 01 December 2010 11:23:53 you wrote:
> On Tuesday 30 November 2010 22:57:29 Charles Manning wrote:
>
> I think I made these comments before, not sure what happened to them...
>
> > +
> > +/* Robustification (if it ever comes about...) */
> > +static void yaffs_retire_block(struct yaffs_dev *dev, int flash_block);
> > +static void yaffs_handle_chunk_wr_error(struct yaffs_dev *dev, int
> > nand_chunk, +					int erased_ok);
> > +static void yaffs_handle_chunk_wr_ok(struct yaffs_dev *dev, int
> > nand_chunk, +				     const u8 * data,
> > +				     const struct yaffs_ext_tags *tags);
> > +static void yaffs_handle_chunk_update(struct yaffs_dev *dev, int
> > nand_chunk, +				      const struct yaffs_ext_tags *tags);
>
> 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.

>
> > +
> > +	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.

> 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.

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

 -- Charles

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