7691 uts: fakebop should not output unprintable data

Review Request #306 — Created Dec. 26, 2016 and submitted

tsoome
illumos-gate
7691
92b0bc9...
general
7691 uts: fakebop should not output unprintable data

boot -B prom_debug=true,kbm_debug=true

  • 0
  • 0
  • 1
  • 1
  • 2
Description From Last Updated
rm
  1. Is there a reason we aren't just printing printable characters and then printing escape sequences for characters we can't print?
    1. I guess we could do that, at the end of the day its just an question which representation to prefer.

      With numbers (like addresses, or sizes) the mixed output does not make any sense, even making things worse. With some other data it may make sense; however it is debug output anyhow, if anyone needs to do some data mining, he/she will most likely just boot to kmdb and dump the memory from there. So, in my opinion, mixed output is just creating confusion.

    2. Hmm. Looking at this again the next day, I somehow missed the bit where we're just printing the whole thing hex escaped. So we're always having the value and not losing it in any way. I think the general strategy is fine. I'll finish the review shortly.
  2. 
      
yuripv
  1. Ship It!
  2. 
      
rm
  1. 
      
  2. usr/src/uts/i86pc/os/fakebop.c (Diff revision 1)
     
     
    Because size is an signed value, we should probably guard against a negative value somehow.
    1. replaced size == 0 with size <= 0. We should not get negative here, but such check does make sense anyhow.

  3. usr/src/uts/i86pc/os/fakebop.c (Diff revision 1)
     
     
    Since this can return a signed value, should we guard against negative values somewhere? bsetprop() doesn't seem to at all. Maybe in the future we should change the property length to an unsigned value here or just error if a negative length is ever set?
    1. I dont think we should in context of this patch; we are fetching the name, value pairs from established property set, the do_bsys_getproplen() will return negative value (-1) in case there is no such property.

      From this api point of view, there is no check on value lenght in bsetprop(), but I think thats probably worth an separate issue.

    2. OK, we can defer that to the future.
  4. 
      
tsoome
rm
  1. Thanks for the follow up changes.

  2. 
      
yuripv
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...