| Message ID | 20181029161534.7564-1-tytso@mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] Fix build failure for discontiguous-io on 32-bit platforms | expand |
On Mon, 2018-10-29 at 12:15 -0400, Theodore Ts'o wrote: > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > --- > src/discontiguous-io.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/discontiguous-io.cpp b/src/discontiguous-io.cpp > index 5e0ee0f..a59c18d 100644 > --- a/src/discontiguous-io.cpp > +++ b/src/discontiguous-io.cpp > @@ -291,7 +291,7 @@ int main(int argc, char **argv) > unsigned char *p = &*buf.begin(); > for (int i = 0; i < len / 4; i++) > iov.append(p + 4 + i * 8, > - std::min(4ul, len - i * 4)); > + std::min(4ul, (unsigned long) len - i * 4)); > } else { > iov.append(&*buf.begin(), buf.size()); > } Hi Ted, Have you considered to change the data type of 'len' from size_t into unsigned long instead of inserting this cast? That would make it clear that no integer truncation happens in the iov.append() call. Thanks, Bart.
On Mon, Oct 29, 2018 at 09:26:43AM -0700, Bart Van Assche wrote: > > Have you considered to change the data type of 'len' from size_t into unsigned long > instead of inserting this cast? That would make it clear that no integer truncation > happens in the iov.append() call. Well the potential integer truncation that could happen is here: case 'l': len = strtoul(optarg, NULL, 0); break; ... but I really don't think the user would ever send a length > 2**32, and I have a sneaking suspicion that ib_srp has zero change of working on 32-bit systems (which is what uses this function) so this was more about making sure blktests would build when I'm building the 32-bit version of my test appliance VM. (For that matter, I've been banging my head against a brick wall trying to make the srp tests work in either a KVM or GCE environment, even with a 64-bit VM, but that's a different issue. It's not high priority for me, at the moment, but eventually I would like the {kvm,gce,android}-xfstest test appliance VM's generated by the xfstests-bld repo to be able to run all of blktests.) If folks have strong opinions about what's the best way to fix this, I'm happy to adjust the patch. (I personally don't care that much). What offends me more is that later on there are scopes when len gets shadowed with a "ssize_t len;" definition --- I whould have thought gcc or clang would have complained bitterly about that, but I guess not. I didn't want to make that many clean up changes, especially since I'm not in a position to test any changes that I make, since as I mentioned the srp blktests are failing miserably for me. - Ted
On Mon, 2018-10-29 at 17:08 -0400, Theodore Y. Ts'o wrote: > On Mon, Oct 29, 2018 at 09:26:43AM -0700, Bart Van Assche wrote: > > > > Have you considered to change the data type of 'len' from size_t into unsigned long > > instead of inserting this cast? That would make it clear that no integer truncation > > happens in the iov.append() call. > > Well the potential integer truncation that could happen is here: > > case 'l': len = strtoul(optarg, NULL, 0); break; If the data type of 'len' would be changed into unsigned long, how could that assignment cause integer truncation since strtoul() returns an unsigned long value? > I have a sneaking suspicion that ib_srp has zero change of > working on 32-bit systems (which is what uses this function) so this > was more about making sure blktests would build when I'm building the > 32-bit version of my test appliance VM. What makes you think that ib_srp won't work on 32-bit systems? I don't think that anyone uses this driver on a 32-bit system. I'm not aware of any aspect of that driver that restricts it to 64-bit systems only either. > (For that matter, I've been banging my head against a brick wall > trying to make the srp tests work in either a KVM or GCE environment, > even with a 64-bit VM, but that's a different issue. It's not high > priority for me, at the moment, but eventually I would like the > {kvm,gce,android}-xfstest test appliance VM's generated by the > xfstests-bld repo to be able to run all of blktests.) Can you be more specific about what didn't work? Were you unable to start the tests or did a particular test fail? > What offends me more is that later on there are scopes when len gets > shadowed with a "ssize_t len;" definition --- I whould have thought > gcc or clang would have complained bitterly about that, but I guess > not. I agree that it would be an improvement to get rid of variable shadowing. Once that has been done the -Wshadow compiler flag can be added. I will look into this if nobody else starts looking into this soon. Bart.
On Mon, Oct 29, 2018 at 02:32:34PM -0700, Bart Van Assche wrote: > > Well the potential integer truncation that could happen is here: > > > > case 'l': len = strtoul(optarg, NULL, 0); break; > > If the data type of 'len' would be changed into unsigned long, how could that > assignment cause integer truncation since strtoul() returns an unsigned long > value? The potential truncation in the existing code is because len is a size_t, and on a 32-bit platform, it's 32 bits while strotul returns a 64-bit value. I also think it really doesn't matter in practice. :-) I'm happy making the len to be an unsigned long, but then I then we would be passing passing a 64-bit to a vector's resize method on a 32-bit platform. It looks like g++ isn't complaining, so sure. I'll resend with that change. > What makes you think that ib_srp won't work on 32-bit systems? I don't think > that anyone uses this driver on a 32-bit system. I'm not aware of any aspect > of that driver that restricts it to 64-bit systems only either. Given the restricted address space on a 32-bit system, I would expect that this might be... challenging for RDMA; and it wasn't clear anyone willing to pay $$$$ to for RDMA gear would be willing to settle for a 32-bit CPU. The fact that support programs for blktests doesn't even build on a 32-bit platform doesn't bode well for how well ib_srp has been tested on 32-bit systems, in any case. :-) > Can you be more specific about what didn't work? Were you unable to start the > tests or did a particular test fail? It took a huge amount of work, but I finally was able to start the tests; but then they *all* fail. Attached is the results of running "kvm-xfstests --blktests srp". Part of the problem is I'm simply not familiar with the userspace tools (or the kernel components for that matter) used by the srp tests. It might simply be that I'm running to some version dependency on the userspace utilities? One thing that would certainly help for newcomers is a minimum set of kernel configs that are needed to allow blktests (and the srp tests in particular) to run. Better yet would be if the blktests listed all of the missing modules, instead of just saying, "you need module <foo>", and then after the user recompiled the kernel with <foo> enabled, then it would say, "you need module <bar>". I must have rebuilt the kernel a dozen+ times, each time needing to figure out which kernel configs I needed to enable to generate the missing module. Eventually when I get this all working, I'll update the suggested kernel defconfigs at https://github.com/tytso/xfstests-bld/tree/master/kernel-configs so others won't have to go through the pain that I did. :-) - Ted P.S. Here are the kernel modules I've found that I've needed for blk-tests on top of kernel defconfigs found above: % grep =m /build/ext4-64/defconfig CONFIG_BLK_DEV_NULL_BLK=m CONFIG_BLK_DEV_NBD=m CONFIG_BLK_DEV_NVME=m CONFIG_NVME_TARGET=m CONFIG_NVME_TARGET_LOOP=m CONFIG_BLK_DEV_SR=m CONFIG_CHR_DEV_SG=m CONFIG_SCSI_DEBUG=m CONFIG_SCSI_DH_RDAC=m CONFIG_SCSI_DH_EMC=m CONFIG_SCSI_DH_ALUA=m CONFIG_BLK_DEV_MD=m CONFIG_MD_MULTIPATH=m CONFIG_DM_MULTIPATH=m CONFIG_DM_MULTIPATH_QL=m CONFIG_DM_MULTIPATH_ST=m CONFIG_TARGET_CORE=m CONFIG_TCM_IBLOCK=m CONFIG_TCM_FILEIO=m CONFIG_TCM_PSCSI=m CONFIG_TCM_USER2=m CONFIG_INFINIBAND=m CONFIG_INFINIBAND_USER_MAD=m CONFIG_INFINIBAND_USER_ACCESS=m CONFIG_INFINIBAND_IPOIB=m CONFIG_INFINIBAND_SRP=m CONFIG_INFINIBAND_SRPT=m CONFIG_RDMA_RXE=m (note the truly impressive number of modules needed in order to run the srp tests :-) CONFIG_LOCALVERSION="-xfstests" CONFIG_SYSVIPC=y CONFIG_POSIX_MQUEUE=y CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y CONFIG_IKCONFIG=y CONFIG_IKCONFIG_PROC=y CONFIG_CGROUPS=y CONFIG_USER_NS=y CONFIG_BLK_DEV_INITRD=y # CONFIG_COMPAT_BRK is not set CONFIG_SLAB=y CONFIG_SMP=y CONFIG_X86_X2APIC=y # CONFIG_X86_EXTENDED_PLATFORM is not set CONFIG_HYPERVISOR_GUEST=y CONFIG_PARAVIRT=y CONFIG_PARAVIRT_SPINLOCKS=y CONFIG_PARAVIRT_TIME_ACCOUNTING=y CONFIG_MCORE2=y CONFIG_NR_CPUS=48 # CONFIG_X86_MCE_AMD is not set # CONFIG_MICROCODE is not set CONFIG_NUMA=y # CONFIG_AMD_NUMA is not set CONFIG_X86_PMEM_LEGACY=y CONFIG_X86_CHECK_BIOS_CORRUPTION=y CONFIG_HZ_300=y CONFIG_KEXEC=y # CONFIG_SUSPEND is not set # CONFIG_ACPI_REV_OVERRIDE_POSSIBLE is not set # CONFIG_ACPI_TABLE_UPGRADE is not set # CONFIG_PCI_MMCONFIG is not set CONFIG_PCI_MSI=y CONFIG_IA32_EMULATION=y # CONFIG_DMIID is not set CONFIG_JUMP_LABEL=y CONFIG_REFCOUNT_FULL=y CONFIG_MODULES=y CONFIG_MODULE_UNLOAD=y CONFIG_MEMORY_HOTPLUG=y CONFIG_MEMORY_HOTREMOVE=y CONFIG_ZONE_DEVICE=y CONFIG_NET=y CONFIG_PACKET=y CONFIG_PACKET_DIAG=y CONFIG_UNIX=y CONFIG_UNIX_DIAG=y CONFIG_INET=y CONFIG_SYN_COOKIES=y # CONFIG_INET_XFRM_MODE_TRANSPORT is not set # CONFIG_INET_XFRM_MODE_TUNNEL is not set # CONFIG_INET_XFRM_MODE_BEET is not set CONFIG_INET_UDP_DIAG=y # CONFIG_INET6_XFRM_MODE_TRANSPORT is not set # CONFIG_INET6_XFRM_MODE_TUNNEL is not set # CONFIG_INET6_XFRM_MODE_BEET is not set CONFIG_NETLINK_DIAG=y # CONFIG_WIRELESS is not set CONFIG_NET_9P=y CONFIG_NET_9P_VIRTIO=y CONFIG_DEVTMPFS=y CONFIG_MTD=y CONFIG_MTD_BLOCK2MTD=y CONFIG_MTD_UBI=y CONFIG_BLK_DEV_NULL_BLK=m CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION=y CONFIG_BLK_DEV_LOOP=y CONFIG_BLK_DEV_NBD=m CONFIG_VIRTIO_BLK=y CONFIG_BLK_DEV_NVME=m CONFIG_NVME_TARGET=m CONFIG_NVME_TARGET_LOOP=m CONFIG_SCSI=y # CONFIG_SCSI_MQ_DEFAULT is not set CONFIG_BLK_DEV_SD=y CONFIG_BLK_DEV_SR=m CONFIG_CHR_DEV_SG=m CONFIG_SCSI_DEBUG=m CONFIG_SCSI_VIRTIO=y CONFIG_SCSI_DH=y CONFIG_SCSI_DH_RDAC=m CONFIG_SCSI_DH_EMC=m CONFIG_SCSI_DH_ALUA=m CONFIG_MD=y CONFIG_BLK_DEV_MD=m CONFIG_MD_MULTIPATH=m CONFIG_BLK_DEV_DM=y CONFIG_DM_SNAPSHOT=y CONFIG_DM_THIN_PROVISIONING=y CONFIG_DM_ZERO=y CONFIG_DM_MULTIPATH=m CONFIG_DM_MULTIPATH_QL=m CONFIG_DM_MULTIPATH_ST=m CONFIG_DM_UEVENT=y CONFIG_DM_FLAKEY=y CONFIG_DM_LOG_WRITES=y CONFIG_TARGET_CORE=m CONFIG_TCM_IBLOCK=m CONFIG_TCM_FILEIO=m CONFIG_TCM_PSCSI=m CONFIG_TCM_USER2=m CONFIG_NETDEVICES=y CONFIG_VIRTIO_NET=y # CONFIG_ETHERNET is not set # CONFIG_WLAN is not set # CONFIG_INPUT_MOUSE is not set # CONFIG_SERIO_SERPORT is not set # CONFIG_LEGACY_PTYS is not set CONFIG_SERIAL_8250=y # CONFIG_SERIAL_8250_DEPRECATED_OPTIONS is not set CONFIG_SERIAL_8250_CONSOLE=y CONFIG_SERIAL_8250_NR_UARTS=32 CONFIG_SERIAL_8250_RUNTIME_UARTS=32 CONFIG_HW_RANDOM=y # CONFIG_HW_RANDOM_INTEL is not set # CONFIG_HW_RANDOM_AMD is not set # CONFIG_HW_RANDOM_VIA is not set CONFIG_HW_RANDOM_VIRTIO=y # CONFIG_HWMON is not set # CONFIG_X86_PKG_TEMP_THERMAL is not set # CONFIG_HID is not set # CONFIG_USB_SUPPORT is not set CONFIG_INFINIBAND=m CONFIG_INFINIBAND_USER_MAD=m CONFIG_INFINIBAND_USER_ACCESS=m CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=y CONFIG_INFINIBAND_IPOIB=m CONFIG_INFINIBAND_SRP=m CONFIG_INFINIBAND_SRPT=m CONFIG_RDMA_RXE=m CONFIG_RTC_CLASS=y # CONFIG_RTC_DRV_CMOS is not set CONFIG_UIO=y CONFIG_VIRT_DRIVERS=y CONFIG_VIRTIO_PCI=y CONFIG_VIRTIO_BALLOON=y # CONFIG_X86_PLATFORM_DEVICES is not set # CONFIG_IOMMU_SUPPORT is not set CONFIG_EXT2_FS=y CONFIG_EXT2_FS_XATTR=y CONFIG_EXT2_FS_POSIX_ACL=y CONFIG_EXT2_FS_SECURITY=y CONFIG_EXT4_FS=y CONFIG_EXT4_FS_POSIX_ACL=y CONFIG_EXT4_FS_SECURITY=y CONFIG_EXT4_ENCRYPTION=y CONFIG_EXT4_DEBUG=y CONFIG_JBD2_DEBUG=y CONFIG_XFS_FS=y CONFIG_XFS_QUOTA=y CONFIG_XFS_POSIX_ACL=y CONFIG_XFS_RT=y CONFIG_BTRFS_FS=y CONFIG_BTRFS_FS_POSIX_ACL=y CONFIG_BTRFS_DEBUG=y CONFIG_BTRFS_ASSERT=y CONFIG_F2FS_FS=y CONFIG_F2FS_FS_SECURITY=y CONFIG_F2FS_CHECK_FS=y CONFIG_F2FS_FS_ENCRYPTION=y CONFIG_FS_DAX=y CONFIG_QUOTA=y CONFIG_QUOTA_NETLINK_INTERFACE=y # CONFIG_PRINT_QUOTA_WARNING is not set CONFIG_QFMT_V2=y CONFIG_AUTOFS4_FS=y CONFIG_OVERLAY_FS=y CONFIG_PROC_KCORE=y CONFIG_PROC_CHILDREN=y CONFIG_TMPFS=y CONFIG_TMPFS_POSIX_ACL=y CONFIG_CONFIGFS_FS=y CONFIG_UBIFS_FS=y CONFIG_UBIFS_FS_ENCRYPTION=y CONFIG_9P_FS=y CONFIG_NLS_DEFAULT="utf8" CONFIG_NLS_ASCII=y CONFIG_NLS_UTF8=y CONFIG_SECURITY=y CONFIG_FORTIFY_SOURCE=y CONFIG_INTEGRITY_SIGNATURE=y CONFIG_IMA=y CONFIG_IMA_WRITE_POLICY=y CONFIG_IMA_APPRAISE=y CONFIG_EVM=y # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set CONFIG_CRYPTO_ECHAINIV=y CONFIG_CRYPTO_CRC32C_INTEL=y CONFIG_CRYPTO_CRC32_PCLMUL=y CONFIG_CRYPTO_AES_NI_INTEL=y # CONFIG_CRYPTO_HW is not set CONFIG_ASYMMETRIC_KEY_TYPE=y CONFIG_SYSTEM_TRUSTED_KEYRING=y CONFIG_SYSTEM_TRUSTED_KEYS="certs/cert.pem" CONFIG_PRINTK_TIME=y CONFIG_DYNAMIC_DEBUG=y CONFIG_DEBUG_INFO=y CONFIG_DEBUG_INFO_REDUCED=y CONFIG_DEBUG_SECTION_MISMATCH=y CONFIG_MAGIC_SYSRQ=y CONFIG_DEBUG_KERNEL=y CONFIG_DEBUG_PAGEALLOC=y CONFIG_DEBUG_OBJECTS=y CONFIG_DEBUG_KMEMLEAK=y CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE=3000 CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF=y CONFIG_DEBUG_STACKOVERFLOW=y CONFIG_HARDLOCKUP_DETECTOR=y CONFIG_WQ_WATCHDOG=y CONFIG_PANIC_TIMEOUT=5 CONFIG_PROVE_LOCKING=y CONFIG_LOCK_STAT=y CONFIG_DEBUG_ATOMIC_SLEEP=y CONFIG_DEBUG_LIST=y CONFIG_DEBUG_SG=y CONFIG_RCU_EQS_DEBUG=y CONFIG_FAULT_INJECTION=y CONFIG_FAIL_MAKE_REQUEST=y CONFIG_FAULT_INJECTION_DEBUG_FS=y CONFIG_FUNCTION_TRACER=y CONFIG_FTRACE_SYSCALLS=y CONFIG_TRACER_SNAPSHOT=y CONFIG_BLK_DEV_IO_TRACE=y CONFIG_FUNCTION_PROFILER=y # CONFIG_RUNTIME_TESTING_MENU is not set CONFIG_DEBUG_WX=y
diff --git a/src/discontiguous-io.cpp b/src/discontiguous-io.cpp index 5e0ee0f..a59c18d 100644 --- a/src/discontiguous-io.cpp +++ b/src/discontiguous-io.cpp @@ -291,7 +291,7 @@ int main(int argc, char **argv) unsigned char *p = &*buf.begin(); for (int i = 0; i < len / 4; i++) iov.append(p + 4 + i * 8, - std::min(4ul, len - i * 4)); + std::min(4ul, (unsigned long) len - i * 4)); } else { iov.append(&*buf.begin(), buf.size()); }
Signed-off-by: Theodore Ts'o <tytso@mit.edu> --- src/discontiguous-io.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)