8093 loader.efi: cleanup loader main and implement comconsole

Review Request #439 — Created April 22, 2017 and submitted

tsoome
illumos-gate
8093
451
2f29b1d...
general
8093 loader.efi: cleanup loader main and implement comconsole


  • 0
  • 0
  • 16
  • 2
  • 18
Description From Last Updated
hans
  1. I'm a bit unhappy about the amount of code duplication between the two serial console implementations, but then I don't really see a nice way to avoid it.

  2. usr/src/boot/sys/boot/efi/loader/comconsole.c (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    It looks like in comc_probe() we don't really care about the actual handles, but only want the number of handles.

    Would it make sense to split getting the bufsz into a separate function for that use case in comc_probe()?

    Wouldn't it also make sense to have a function efi_serial_free() to free the handle array?

    Are the handles even used anywhere?

    1. actually the comc_probe() does open protocol with that handle (line 189), the protocol is stored in port private pointer (port->sio) and the protocol is used for read/write and attribute management. Once we have the protocol, we wont need the handles any more. As the handle array was allocated with simple malloc(), I do not see mutch reason for efi_serial_free() as it would be simply call to free() anyhow... note the majority of UEFI API calls do get the buffer size in the same way as here - you make call with NULL buffer and pointer to size, receive error EFI_BUFFER_TOO_SMALL and the size is set for expected buffer size, then allocate the buffer and repeat the call.

  3. I'd add some spaces around the ':'.

  4. Spaces around ':"?

  5. The name is a bit confusing as you're not really printing anything. Perhaps com_sprint_mode() or com_asprint_mode() would be better.

  6. Please use snprintf(). Either pass the buffer size as an additional argument, or make this function allocate the buffer.

    Actually making this function allocate the buffer itself would be preferable, as that would rule out any chance for truncation.

  7. I think here and in other places where you parse those numbers you should explicitly use a base of 10, and perhaps use strtoul() as those values cannot be negative anyway.

  8. usr/src/boot/sys/boot/efi/loader/main.c (Diff revision 1)
     
     

    What does this magic number do?

    1. the string length; but, I did double check the specification and with EFI_SUCCESS the data is not valid there:

      "ResetData is only valid if ResetStatus is something other than EFI_SUCCESS unless the ResetType is EfiResetPlatformSpecific where a minimum amount of ResetData is always required."

  9. usr/src/boot/sys/boot/i386/libi386/comconsole.c (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    While here, can you please fix this the same way as in the EFI code?

  10. Please fix this (and a few lines down) the same way as in the new EFI code.

  11. 
      
rm
  1. 
      
  2. Is there a reason this is a signed value? If it wasn't then we don't have to cast at +134 or worry about overflow. I'm not sure if we assign it to a negative value ever.

    1. The background is actually quite simple - we get the list of 4 tty entries (ttya-ttyd) from /boot/solaris/bootenv.rc, but we do not know at that point if all 4 are actually usable. So I am recording the port index, and if there are less than 4 ports, I set the index -1 and since I test the index value with the number of handles we get from efi_serial_init(), I should not compare signed versus unsigned value.

      Also from another hand, indeed, the handle count itself can only be positive, no question there. So can it be if we end up overflowing the signed int as an result of bufsz/sizeof (EFI_HANDLE) operation. The EFI_HANDLE is actually void *, that is, 4 or 8 bytes (EFI 32/64).

      If we assume for moment, that UEFI API itself is doing the work right, then we should not see that large bufsz, simply because there can not be thousands of serial ports in system.

      If by some reason the LocateHandle() will give us back very large bufsz value, then the malloc() will fail, because we have 64MB heap for malloc, and we return with ENOMEM. So I think we are quite safe from both logical point of view (not that many real serial ports), and the memory allocation will set the natural limit there too.

  3. Use sizeof (name), rather than hardcoding the size.

  4. Use sizeof (name).
  5. Use sizeof (name).

  6. We have a size for value, so let's use snprintf()?

  7. I agree with Hans that we want to use snprintf().

  8. Also to properly check for errors in strtol you probably need to set errno to zero initially or at least check the return value of it to verify that it's not ERANGE.
  9. Same comments on correctly checking errno.

  10. What guarantees that name is long enough to index here?

    1. the get_console is called by comc_mode_set(), comc_cd_set() and comc_rtsdtr_set(), which are callback functions set up by env_setenv() in comc_probe(). The callback is registered for specific environment variable, in this case "ttyX-mode", "ttyX-ignore-cd" and "ttyX-rts-dtr-off". So whenever user/script will set/change the value for this specific variable, the callback is called with the variable name. As we do set up those specific callbacks for specific variables, for ports [a-d], we do get not only long enough name, but the correct name.

  11. usr/src/boot/sys/boot/efi/loader/main.c (Diff revision 1)
     
     
    Should there be a separate bug for this?
    1. Actually yes, you are right, this update was part of larger commit and obviously I have missed to extract it, I will move this one into separate update.

  12. 
      
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/efi/loader/comconsole.c (Diff revisions 1 - 2)
     
     

    I'm not sure this is right, because when n is zero we still need a byte to print that zero.

  3. usr/src/boot/sys/boot/efi/loader/comconsole.c (Diff revisions 1 - 2)
     
     
     
     
     
     
     
     
    Actually rather than this guessing game, why not just use snprintf(NULL, 0, ...) and that'll give you the buffer size you need.
    1. It actually is not that simple, because in libstand they have done the implementations assuming non-NULL buffer:D

      Anyhow, I did create asprintf() as pre-patch, and that will simplify it all and we can get nice and clean code.

  4. usr/src/boot/sys/boot/efi/loader/comconsole.c (Diff revisions 1 - 2)
     
     
    Where exactly does 10 come from? That doesn't seem to add up with the combination of the %ju, the commas, and '-' character. Though maybe my math is off?
    1. Actually the %ju was counted above, so we only need to count ",%d,%c,%d,-", and all numbers are 1 char, so with terminating 0 we need 9;) but yes, it is much better not to count here and just let asprintf() to do its work.

  5. I think this has the same logic questions that I asked about up above. Though you may be able to get away with the +10 here. But I think the snprintf will be even simpler.

  6. 
      
tsoome
tsoome
hans
  1. Ship It!
  2. 
      
rm
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...