On Tue, Dec 11, 2018 at 03:20:23PM -0500, Nick Bowler wrote: > On 12/11/18, Nick Bowler <nbowler@xxxxxxxxxx> wrote: > > On 12/11/18, Brian Foster <bfoster@xxxxxxxxxx> wrote: > >> Given that the structure size essentially changes the command value, I'm > >> kind of curious why we have this ifdeffery in the first place. That > >> aside, the patch seems reasonable to me at a glance (though some brief > >> comments around the ifdefs would be nice). > [...] > > Current code has the ifdeffery. Also since it's a syntax error to have > > multiple case labels with the same value it'd be essential to validate > > that all supported architectures architectures end up with different > > values for each XFS_IOC_xyz and the corresponding XFS_IOC_xyz_32. > > Right after I write this, I realize that it's almost certainly > the case that architectures which _don't_ define BROKEN_X86_ALIGNMENT > will have matching ioctl numbers between e.g., XFS_IOC_ALLOCSP and > XFS_IOC_ALLOCSP_32. Thus the ifdeffery is essential for the above > syntactic reason. > So the two struct [compat_]xfs_flock64 variants are essentially the same with the exception of internal alignment padding that leaves a hole after the first 4 bytes in the 64-bit variant. So on x86_64, these are essentially two different (sized) structures and ioctl commands. On some non-broken alignment arch, the packing may be implied and thus these definitions evaluate down to the same ioctl command. If so, then having those separate but equivalent value commands in the same switch statement results in a compilation error. Am I following that correctly? If so, then it does seem we need an ifdef to exlude those definitions so long as we follow the one huge switch statement implementation. But is there anything that fundamentally prevents a multiple switch statement implementation to avoid such syntax errors? Note that in the end I don't care too much about whether we have an ifdef or not. I'm more interested in the most simple and elegant implementation and it just seemed that trying to dilute the existing ifdef may push us in the opposite direction (as opposed to something that for example called into the "native" ioctl and fell back to the _32 variants on failure before returning an error). There may be technical complications to that or using some form of the ifdef may just end up being the more simple approach after all. Brian > Cheers, > Nick
