8369 libcmdutils should be better about large file support / 8370 libcmdutils needlessly defines its own OFFSETOF() macro

Review Request #575 — Created June 11, 2017 and submitted

jbk
illumos-gate
general

Allows 32-bit consumers to link against it without making lint irate

Ran full nightly, verified no errors

  • 0
  • 0
  • 0
  • 4
  • 4
Description From Last Updated
yuripv
  1. 
      
  2. usr/src/lib/libcmdutils/libcmdutils.h (Diff revision 1)
     
     

    Ugh, here and below, shouldn't this be "if NOT (_FILE_OFFSET_BITS == 64)"?

    1. Oh, I get it, yet another lint gag - probably worth a comment so we drop all this along with lint? :-)

    2. Well not so much a gag as libcmdutil's largefile support (interface) was arguably incomplete. Even if consumers weren't using the relevant bits of libcmdutils (i.e. writefile() or the tnode stuff) and didn't care about it's largefile support at all, lint would still complain if the consumer was 32-bit & _LARGEFILE64_SOURCE.

  3. 
      
tsoome
  1. Ship It!
  2. 
      
jbk
  1. 
      
  2. usr/src/lib/libcmdutils/libcmdutils.h (Diff revision 1)
     
     

    I don't think so -- _FILE_OFFSET_BITS redefines the types in 32-bit mode as the 64-bits (i.e. ino_t w/ _FILE_OFFSET_BITS == 64 is 64bits) _LARGEFILE64_SOURCE defines the xxx64 versions -- i.e. w/ _LARGEFILE64_SOURCE in 32-bit mode off_t is 32-bits, while off64_t is 64-bits.

    The library itself is built with _FILE_OFFSET_BITS=64, so the 32-bit version is largefile capable.

  3. 
      
yuripv
  1. Ship It!
  2. 
      
rm
  1. Ship It!
  2. 
      
gwr
  1. 
      
  2. usr/src/lib/libcmdutils/libcmdutils.h (Diff revision 1)
     
     

    So long as all the data structures defined in here are explicit about using the "large file" version of types (i.e. ino64_t etc.) can't this be used independent of the compilation environment? What am I missing? (or forgetting:)

    1. Unfortunately, then lint again gets angry because app is using 'ino_t' and the header file is 'ino64_t' -- so that'd mean updating all the consumers of libcmdutils to use the xxx64 version of stuff -- I tried that, but it just ended up being a mess (usr/src/cmd/mv/mv.c was probably the worst), and just introduced a lot of churn.

    2. Oh, that's too bad. Perhaps add a comment somewhere (the header?) about why the CPP conditional?
      Otherwise it's a conditional that end up with the same base type either way...
      But yeah, I'm fine with you leaving the rest of that mess for another day:)

    3. Another thought just occured to me:
      Would this be an appropraite place for an #ifdef lint hack?
      (would that be any better? not sure)

  3. usr/src/lib/libcmdutils/libcmdutils.h (Diff revision 1)
     
     

    Did you consider just always defining this structure using ino64_t instead of the cpp conditionals?

    Or would that cause compatibility problems?
    Maybe that's what I'm missing in this change...
    If so, a little more detail in the issue would help.
    Thx!

    1. Yes -- see above.

  4. 
      
jbk
Review request changed

Status: Closed (submitted)

Loading...