8797 loader: Support URI scheme for root-path in netbooting

Review Request #732 — Created Nov. 11, 2017 and submitted

tsoome
illumos-gate
8797
14e3ad1...
general
https://reviews.freebsd.org/D10947
https://svnweb.freebsd.org/base?view=revision&revision=318999


  • 0
  • 0
  • 5
  • 1
  • 6
Description From Last Updated
rm
  1. 
      
  2. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 1)
     
     
    Maybe instead of ip it should be IPv4? Since this seems like it only handles IPv4 addresses.
    1. Yes, we do not support IPv6 (yet).

  3. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 1)
     
     
     
     
     

    I find these couple of sentences a little confusing. Is this comment correct?

    ```
    If an IPv4 address has been specified, it will be stripped out and passed out as the return value of this function in network byte order.

    If no global default scheme has been specified and no scheme has been specified, we will assume that this is an NFS URL.

    The pathname will be stored in the global variable rootpath.

    1. Yep, I think this is much better wording:)

  4. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 1)
     
     
     
    Should this also cover IPv6?
  5. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 1)
     
     
    Shouldn't we check if this is larger than FNAME_SIZE and if so, not actually pass it to inet_addr as it may be truncated?
    
    I assume that the reason we're trying to do it this way and not set the strchr result to '\0' is because we don't want to modify this?
    1. Since inet_addr() will return INADDR_NONE on error, I think we have sufficient error processing below (after the fix from next issue).

      The rootpath will get modified eventually anyhow as the code will remove the scheme and IP if present, but I guess the original author felt safer to use this approach. Seems to work, so I think we can have it as is for now.

  6. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 1)
     
     
    If the parsed address is INADDR_NONE should we be warning about that? How does a user tell the difference between having no IP address specified, versus having a garbage IP address here. For example, if this includes the port, then that'll be there.
    1. Did add diagnostic printout.

  7. usr/src/boot/sys/boot/common/dev_net.c (Diff revision 1)
     
     
    Is the reason for this that we want to capture the initial '/' character that's in the URI if none is there?
    1. Yes, because this code does allow <scheme>:/path without an IP (when we use DHCP server IP for example), then we do like to have full path with starting '/'.

  8. 
      
tsoome
tsoome
rm
  1. Thanks for the follow ups.

  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...