Re: [PATCH] fs/befs/linuxvfs.c: need signed cast for variable 'block'

[Chen Gang <gang.chen@asianux.com>]

On 11/03/2013 12:27 AM, Al Viro wrote:
> On Sat, Nov 02, 2013 at 08:44:46AM -0700, Greg KH wrote:
> 
>>> Oh, for me, it is not suitable to move a file system sub-directory to
>>> "drivers/*/" sub-directory. And I can not find any sub-directory like
>>> 'staging' under "fs" sub-directory, either.
>>>
>>> Do we have any sub-directory like "staging" in "fs" sub-directory? if
>>> no, do we have to create it or have to use another ways instead of?
>>
>> Just move the filesystem to drivers/staging/befs.
> 
> Actually, having read through that code...  It's not too scary; r/w
> support would've been much more hairy, but this is just r/o.  We
> probably don't need to move that sucker at all.
> 

After a quick look, I guess we can not remove it from Linux kernel:

 - according to "fs/befs/Kconfig",
   it is only for support file system from BeOS.

 - according to "http://en.wikipedia.org/wiki/BeOS",
   BeOS is still in use (although it is almost dead).

 - according to "git log -p fs/befs",
   it is still be changed recently.

Can any members provide more proofs for it (my proofs are not enough).
if it is still using, we have to keep it in Linux kernel, else support
to remove it to some where firstly (but not "drivers/*").


> As for befs_get_block(), I'd suggest
> 	* taking the range checks for block number into its ->bmap()
> (just check against the file size and return 0 if it doesn't fit)

befs_get_block() is called both by befs_bmap() and by befs_readpage(),
and I guess Al's "->bmap()" points to befs_bmap() -- if what I guess is
incorrect, please let me know, thanks.

So for me, just remove this check "if (block < 0) { ... }" is OK which
is Kees said.

And for me, need use "%llu" not "%lu" instead of "%ld" (according to
"Documentation/printk-format.txt").


> 	* turning the check for create != 0 into BUG_ON(create)

Can we be sure of:

 - in current case, kernel/sub-system must be continuing incorrectly.
 - and if let it continue, it will cause critical issue.

If:

  can be sure of both, use BUG_ON().
  can be sure of 1st item, use WARN_ON().
  can be sure of neither, keep original no touch.

For me, if what Kees said is correct (for "ioctl ... printks ..."), I
support to use WARN_ON() which indicates "the sub-system is continuing
incorrectly, although it not a critical issue".


> 	* making the befs_fblock2brun() failure quiet - the sucker
> will complain itself, just return that -EFBIG and be done with that.
> 

Do you mean befs_fblock2brun() must be OK (should check it previously)?
If not, we cann't skip the failure -- next iaddr2blockno() may cause panic.

And for me, use "-EFBIG" is really not quite smart.

Hmm... the same reason as above (if "ioctl ... printks ..." correct),
may we need WARN_ON(), and then still return "-EFBIG"?.



BTW: the local variable 'res' can be removed.



Thanks.
-- 
Chen Gang