On 07/31/2014 05:13 PM, Matthew Wilcox wrote: > On Thu, Jul 31, 2014 at 01:11:42PM +0300, Boaz Harrosh wrote: >>>>> + if (size < 0) >>>> >>>> if(size < PAGE_SIZE), No? >>> >>> No, absolutely not. PAGE_SIZE is unsigned long, which (if I understand >>> my C integer promotions correctly) means that 'size' gets promoted to >>> an unsigned long, and we compare them unsigned, so errors will never be >>> caught by this check. >> >> Good point I agree that you need a cast ie. >> >> if(size < (long)PAGE_SIZE) >> >> The reason I'm saying this is because of a bug I actually hit when >> playing with partitioning and fdisk, it came out that the last partition's >> size was not page aligned, and code that checked for (< 0) crashed because >> prd returned the last two sectors of the partition, since your API is sector >> based this can happen for you here, before you are memseting a PAGE_SIZE >> you need to test there is space, No? > > Not in ext2/ext4. It requires block size == PAGE_SIZE, so it's never > going to request the last partial block in a partition. > OK cool. then. Matthew what is your opinion about this, do we need to push for removal of the partition dead code which never worked for brd, or we need to push for fixing and implementing new partition support for brd? Also another thing I saw is that if we leave the flag GENHD_FL_SUPPRESS_PARTITION_INFO then mount -U UUID stops to work, regardless of partitions or not, this is because Kernel will not put us on /proc/patitions. I'll submit another patch to remove it. BTW I hit another funny bug where the partition beginning was not 4K aligned apparently fdisk lets you do this if the total size is small enough (like 4096 which is default for brd) so I ended up with accessing sec zero, the supper-block, failing because of the alignment check at direct_access(). Do you know of any API that brd/prd can do to not let fdisk do this? I'm looking at it right now I just thought it is worth asking. Thanks for everything Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
