7554 libstand: add interface-mtu option

Review Request #259 — Created Nov. 9, 2016 and submitted

tsoome
illumos-gate
7554
06ecff3...
general
7554 libstand: add interface-mtu option


  • 0
  • 0
  • 6
  • 1
  • 7
Description From Last Updated
andy_js
  1. Ship It!
  2. 
      
tsoome
andy_js
  1. Ship It!
  2. 
      
rm
  1. 
      
  2. usr/src/boot/lib/libstand/bootp.c (Diff revision 2)
     
     
    Should we really ignore all failures here or not even warn the user that something bad was passed?
  3. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 2)
     
     
    Can we use snprintf?
  4. Can we use snprintf?
  5. 
      
tsoome
rm
  1. 
      
  2. usr/src/boot/lib/libstand/bootp.c (Diff revisions 1 - 3)
     
     
    Previously this was in an else block. But now we're purposefully using it. Can you explain a bit more about the intended logic?
    1. The previous code did not check the result from strtol, it could be an error from conversion or 0, and 0 is obviously not valid value for MTU. So my idea was, if there is an error in conversion, set intf_mtu to 0 and use value from dhcp response and ignore the value suggested in environment.

      Hm, it just did occur me, I actually should use temporary signed variable there and exclude negative values as well... and intf_mtu <= 0 is just a bit too much as intf_mtu is unsigned...

    2. I think that might be a reasonable fallback plan here.
  3. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 3)
     
     
    Does FBSD use the same convention of a space after the sizeof()?
    1. Unfortunately no - their cstyle is set to use space after keywords, except sizeof, and its kind of problem at least for me - I have got bitten by it already;) However, I dont mind adding space there to follow our case...

    2. Given that they're the upstream for this, seems that we probably should match.
  4. 
      
tsoome
rm
  1. 
      
  2. usr/src/boot/lib/libstand/bootp.c (Diff revision 4)
     
     
    Sorry I didn't notice this sooner, but is casting this correct? This means that we'll truncate the value right?
    
    So something that's say UINT_MAX + 1 will become UINT_MAX. Should we just treat anything greater than UINT_MAX as a bad value? I mean, anything really larger than 64k is probably impossible to use given the fact that this is IP which has a 64K max. But it seems like if there is clamping to be done, it's not here.
    1. Well, if the string is presenting value too large, we will get ULONG_MAX and errno is set ERANGE - which means the intf_mtu is reset to 0 as errno != 0. For max value... today it seems max jumbo is 9600 on some hardware, so from practical point anything >10000 is most likely not working anyhow. I dont know if we really should check the upper value here or not, as essentially setting anything not used by your switch, is shooting yourself into the leg:)

    2. I'm describing a slightly different case. Anything in the range of [UINT_MAX + 1, ULONG_MAX] is a valid return value from strtoul and will not result in setting errno to zero. However, it will then be truncated, and not to UINT_MAX, but IIRC, we'll just cut off the upper 32 bits (assuming LP64). Maybe this is all 32-bit code that's assuming an ILP32 environment, but it still seems like you could easily be truncating valid values without getting ERANGE.
  3. 
      
tsoome
rm
  1. 
      
  2. usr/src/boot/lib/libstand/bootp.c (Diff revisions 4 - 5)
     
     
    The IP MTU is 64K in base 2. So 64 * 1024.
    1. max value of 16-bit value, 0xffff, to be exact, for IPv4.

  3. usr/src/boot/lib/libstand/bootp.c (Diff revisions 4 - 5)
     
     
    This should have a cast for LP64 completeness.
  4. 
      
tsoome
rm
  1. Ship It!
  2. 
      
andy_js
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...