8374 loader: devicename.c cleanup

Review Request #577 — Created June 12, 2017 and submitted

tsoome
illumos-gate
8374
547
089e0eb...
general
8374 loader: devicename.c cleanup


  • 0
  • 0
  • 3
  • 1
  • 4
Description From Last Updated
rm
  1. 
      
  2. I'm not sure if this will still trigger correctly. Previously this relied on dv being only assigned during the if statement. However, in this case because the for loop doesn't assign dv, you'll end up assigning dv during the last iteration of the devsw[i] being valid and then when devsw[i] is not null, dv won't get assigned NULL thus breaking the logic of this check.

    I think either you need to move it so that the dv assignment is under the if or change the check to use devsw[i] and assign dv after the loop and if check.

    1. ah, indeed it is bug, checking devsw[i] == NULL is quite enough - if it is NULL, we are done anyhow. if its not NULL, we have good dv, as we did break off from the loop.

  3. Is there a reason we're casting away the constness of np? Should np have been const to begin with?
    1. Unless I miss something, it really is about cast one way or another. The reason np is const, is about devspec argument - so if np is not const, we would need the cast at line 105, where we set np = devspec + strlen(dv->dv_name); if *cp is const, then we would need cast with strtol().

    2. Yeah, I think to me the thing then should be is that we use a separate var for one case or the other. I get why they used it that way, but basically whenever we remove constness, that feels like a hack.

  4. Should this be checking against 0 or '\0'?
  5. Now that the buffer size is based on a macro, should we instead use strlcpy here just to play it safe?

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

Status: Closed (submitted)

Loading...