10009 btxld: handle versioning better

Review Request #1297 — Created Nov. 26, 2018 and submitted

tsoome
illumos-gate
10009
1286
8771076...
general
10009 btxld: handle versioning better

build and verify the generated binaries with installboot -i

  • 1
  • 0
  • 2
  • 2
  • 5
Description From Last Updated
Nested ternary conditional operator (c-style issue), could be: switch (i) { case I_LDR: fname = lname; break; case I_BTX: fname ... domag02 domag02
rm
  1. 
      
  2. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     
    Do we need the explicit cleanup here? Isn't it going to be called via the atexit?
    1. Well yes. I just figured that since the cleanup code was there, it might be good to keep it around - if for some reason we need to add some more activities, then we do not have to start to invent the cleanup...

    2. OK, given that cleanup should be idempotent it should be OK I guess.
  3. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     
    It looks like this was always missing, right?
    1. moved there from btxld(), as the original tempname cleanup was there.

  4. 
      
domag02
  1. Standard macro constant for maximum unsigned 32-bit quantity.
  2. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     

    Unnecessary, use UINT32_MAX from <stdint.h> instead.

  3. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     

    Should be:

    if (sb.st_size > UINT32_MAX)
    
  4. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     

    Should be:

    if (errno || !*arg || *s || x > UINT32_MAX)
    
  5. 
      
domag02
  1. Space-tab issues.
  2. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     
    Unnecessary space before tab.
  3. usr/src/tools/btxld/btxld.c (Diff revision 1)
     
     
    Same (+next line).
  4. 
      
tsoome
rm
  1. Ship It!
  2. 
      
domag02
  1. Unnecessary format string variables.
  2. usr/src/tools/btxld/btxld.c (Diff revision 2)
     
     

    I think these variables (binfo, cinfo, oinfo) unnecessary, because these format strings used only once (and far away).

  3. usr/src/tools/btxld/btxld.c (Diff revision 2)
     
     

    Could be:

        printf("kernel: ver=%u.%02u size=%x load=%x entry=%x map=%uM "
            "pgctl=%x:%x\n", btx.btx_majver, btx.btx_minver, btx.btx_textsz,
            BTX_ORIGIN(btx), BTX_ENTRY(btx),
            BTX_MAPPED(btx) * BTX_PGSIZE / 0x100000,
            !!(btx.btx_flags & BTX_MAPONE),
            BTX_MAPPED(btx) - btx.btx_pgctl -
            BTX_PGBASE / BTX_PGSIZE - BTX_MAPPED(btx) * 4 / BTX_PGSIZE);
        printf("client: fmt=%s size=%x text=%x data=%x bss=%x entry=%x\n",
            fmtlist[ihdr.fmt], ihdr.size, ihdr.text, ihdr.data, ihdr.bss,
            ihdr.entry);
        printf("output: fmt=%s size=%x text=%x data=%x org=%x entry=%x\n",
            fmtlist[ohdr.fmt], ohdr.size, ohdr.text, ohdr.data, ohdr.org,
            ohdr.entry);
    
  4. This expression could be a good candidate for an another function-like macro:

    BTX_MAPPED(btx) - btx.btx_pgctl - BTX_PGBASE / BTX_PGSIZE - BTX_MAPPED(btx) * 4 / BTX_PGSIZE
    
domag02
  1. 
      
  2. usr/src/tools/btxld/btxld.c (Diff revision 2)
     
     

    Nested ternary conditional operator (c-style issue), could be:

        switch (i) {
            case I_LDR:
                fname = lname; 
                break;
            case I_BTX:
                fname = bname;
                break;
            default:
                fname = iname;
                break;
        }
    
  3. 
      
domag02
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...