On Tue, 28 Oct 2014 16:47:06 -0700 Justin Maggard <jmaggard10@xxxxxxxxx>
wrote:
> On Tue, Oct 28, 2014 at 3:19 PM, NeilBrown <neilb@xxxxxxx> wrote:
> > On Fri, 24 Oct 2014 17:55:02 -0700 Justin Maggard <jmaggard10@xxxxxxxxx>
> > wrote:
> >
> >> If the request --size to --grow an array to is larger
> >> than 32bits, then mdadm may make the wrong choice and
> >> use ioctl instead of setting component_size via sysfs
> >> and the change is ignored.
> >
> > Can you explain exactly why the current code is not sufficient? When does
> > it fail?
> > If you include the explanation in a re-submission of the patch, and I am
> > convinced, then I will gladly apply your patch.
> >
>
> To be honest, I can't figure out how the current comparison would have
> ever worked at all. To illustrate, here's a simple test program:
> -----
> $ ./test 0xfffffffff
> cast comparison did not detect truncation
> bit comparison detected truncation
>
> $ cat test.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdint.h>
>
> int main(int argc, char **argv)
> {
> unsigned long long ssize = strtoull(argv[1], NULL, 0);
> int asize;
>
> asize = ssize;
> if (asize != (signed)ssize)
> printf("cast comparison detected truncation\n");
> else
> printf("cast comparison did not detect truncation \n");
> if (ssize & ~INT32_MAX)
> printf("bit comparison detected truncation\n");
> else
> printf("bit comparison did not detect truncation \n");
>
> return 0;
> }
> -----
>
> I plugged lots of numbers in there, and I was never able to get the
> current cast comparison to see a difference.
>
> I ran into the issue by trying to grow the component size of a RAID
> array from 1TB to 3TB, and it wouldn't work if I specified the size;
> only using "max" worked.
>
> I'm happy to re-submit if you'd like; I just thought it was a pretty
> straightforward bug. I guess what I'm saying is, I don't understand
> why there *should* be a difference between assigning a unsigned long
> long to an int variable, and casting that unsigned long long to a
> signed type. But
>
> -Justin
>
> > Thanks,
> > NeilBrown
> >
> >>
> >> Instead of using casts to check for a 32-bit overflow,
> >> just check for set bits outside of INT32_MAX.
> >> ---
> >> Grow.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/Grow.c b/Grow.c
> >> index a9c8589..a614102 100644
> >> --- a/Grow.c
> >> +++ b/Grow.c
> >> @@ -1818,7 +1818,7 @@ int Grow_reshape(char *devname, int fd,
> >> if (s->size == MAX_SIZE)
> >> s->size = 0;
> >> array.size = s->size;
> >> - if (array.size != (signed)s->size) {
> >> + if (s->size & ~INT32_MAX) {
> >> /* got truncated to 32bit, write to
> >> * component_size instead
> >> */
> >
Thanks.
Looks like I broke it in July.
http://git.neil.brown.name/?p=mdadm.git;a=commitdiff;h=4e9a3dd16d656b269f5602624ac4f7109a571368
I probably should have made it
if (s->size != (signed long long)s->size)
I've applied your patch.
Thanks,
NeilBrown
Attachment:
pgpIIYyrKHhOB.pgp
Description: OpenPGP digital signature
