linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: do_div considered harmful
@ 2003-08-04  5:31 Steve French
  0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2003-08-04  5:31 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Andries.Brouwer, linux-kernel, torvalds

I have made a change to fix this in fs/cifs/inode.c and a similar 
problem in fs/cifs/file.c
(and a change to fs/cifs/cifsfs.c to better match the blocksize bits and 
blocksize default).
It is (version 0.8.7 of the cifs vfs) in 
bk://cifs.bkbits.net/linux-2.5cifs and I am testing it now.

 >> Similarly, I find in fs/cifs/inode.c>
 >>          inode->i_blocks = do_div(findData.NumOfBytes, 
inode->i_blksize);
 >
 >This should be
 >
 >        int blocksize = 1 << inode->i_blkbits;
 >
 >         inode->i_blocks = (findData.NumOfBytes + blocksize - 1)
 >                                    >> inode->i_blkbits;


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: do_div considered harmful
@ 2003-08-04  9:46 Andries.Brouwer
  0 siblings, 0 replies; 5+ messages in thread
From: Andries.Brouwer @ 2003-08-04  9:46 UTC (permalink / raw)
  To: Andries.Brouwer, akpm; +Cc: linux-kernel, torvalds

> Sometimes the slash-star operator comes in handy.

Certainly an improvement. Maybe it will help a little.

But really, do_div is not a C function, it has very
nonintuitive behaviour.
The two bugs are: (i) the name is wrong, it returns a
remainder but the name only talks about dividing, and
(ii) worst of all, it changes its first argument.

> + * uint32_t do_div(uint64_t *n, uint32_t base)

And the comment that you added, copied from elsewhere,
is confusing too. The first parameter is not uint64_t *.
This thing cannot be described in C.

I would still like to replace do_div by DO_DIV_AND_REM
as a big fat warning - this is a macro, read the definition.
And the common case, where no remainder is used, by DO_DIV
#define DO_DIV(a,b)	(void) DO_DIV_AND_REM(a,b)

Andries

[Perhaps DO_DIV64, to also indicate a type.]

[The same problem happens with sector_div. I recall having
to fix sector_div calls in scsi code. These functions are
bugs waiting to happen. Nobody expects that after
	res = func(a,b);
the value of a has changed.]

^ permalink raw reply	[flat|nested] 5+ messages in thread
* do_div considered harmful
@ 2003-08-04  2:03 Andries.Brouwer
  2003-08-04  2:29 ` Andrew Morton
  2003-08-04  5:54 ` Linus Torvalds
  0 siblings, 2 replies; 5+ messages in thread
From: Andries.Brouwer @ 2003-08-04  2:03 UTC (permalink / raw)
  To: linux-kernel; +Cc: torvalds

Writing this ide capacity patch an hour ago or so
I split off a helper sectors_to_MB() since Erik's recent
patch uses this also.
Now that I compare, he wrote
	nativeMb = do_div(nativeMb, 1000000);
to divide nativeMb by 1000000.
Similarly, I find in fs/cifs/inode.c
	inode->i_blocks = do_div(findData.NumOfBytes, inode->i_blksize);

So, it seems natural to expect that do_div() gives the quotient.
But it gives the remainder.
(Strange, Erik showed correct output.)

Since the semantics of this object are very unlike that of a C function,
I wonder whether we should write DO_DIV instead, or DO_DIV_AND_REM
to show that a remainder is returned.

Andries

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2003-08-04  9:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-08-04  5:31 do_div considered harmful Steve French
  -- strict thread matches above, loose matches on Subject: below --
2003-08-04  9:46 Andries.Brouwer
2003-08-04  2:03 Andries.Brouwer
2003-08-04  2:29 ` Andrew Morton
2003-08-04  5:54 ` Linus Torvalds

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