8140 loader: network read rework

Review Request #462 — Created May 1, 2017 and submitted

461, 476

We need to allocate buffer while fetching data from the nic and
pass it up the stack, not vice versa.

The FreeBSD update: https://reviews.freebsd.org/rS317887

  • 0
  • 0
  • 2
  • 4
  • 6
Description From Last Updated
  2. usr/src/boot/lib/libstand/ether.c (Diff revision 2)
    Don't we need to check if this is a VLAN tagged frame? As if it's not, this payload member may be incorrect (and thus a bunch of others).
    1. The current code is assuming untagged, we do not support vlan this time; however, thinking about it, it should not be too hard to implement, but I think it would be better to be left for separate issue.

  3. usr/src/boot/lib/libstand/nfs.c (Diff revision 2)
    Shouldn't this return rc?
  4. usr/src/boot/lib/libstand/nfs.c (Diff revision 2)

    I assume this was removed as part of other cleanup?

    1. The cleanup is in fact part of this update; firstly, using dv_type is much better for this check and secondly, the BIOS specific "pxe" is removed and replaced by generic "net" device. The BIOS implementation was using pxe UDP api and not more generic UNDI.

  5. usr/src/boot/lib/libstand/rpc.c (Diff revision 2)
    Does recvrpc actually guarantee that we have the space here? I guess this is the 4 * 4 down below?
    1. we check for reply size we get from sendrecv() at line 178.

  6. usr/src/boot/lib/libstand/tftp.c (Diff revision 2)

    Do we not need to free the pkt in these cases? Or is the assumption that once we get to the point at +345, it's someone else's responsibility? Maybe it'll all funnel to tftp_close()?

    1. Yep, we should free it on tftp_close(). However, there is potential leak as the handle's pkt can get set, but if we return to tftp_open() with failure, then the pkt will get leaked when handle is released.

  7. usr/src/boot/lib/libstand/udp.c (Diff revision 2)
    While we fix up the ip_len here, shouldn't we be fixing up the checksum? At this point, won't it be wrong?
    1. I think you are right there. I actually do not like that ip options removal - I think with this rewrite it should not be needed, however since this code has been there, I think it would be better to address this in separate issue, or we are overcomplicating this change:)

  1. Ship It!
Review request changed

Status: Closed (submitted)