7242 usr/src/cmd/boot build needs cleanup

Review Request #210 — Created July 28, 2016 and submitted

tsoome
illumos-gate
7242
general
7242 usr/src/cmd/boot build needs cleanup


  • 0
  • 0
  • 0
  • 3
  • 3
Description From Last Updated
tsoome
igork
  1. 
      
  2. usr/src/cmd/boot/bootadm/bootadm.c (Diff revision 2)
     
     

    what we check check here !=0 ? maybe check to != BAM_SUCCESS ?

    1. ret is also used to accumulate return result from nvlist functions, so its not correct to use macro there.

  3. 
      
jeffpc
  1. 
      
  2. usr/src/cmd/boot/mbr/mbr.c (Diff revision 2)
     
     

    C-style says to use == 0 for ints.

    1. True, but also almost every if statement in this file has same issue, and since gcc & pbchk are not complaining, rewriting the mbr.c is going to be out of the scope of this change. Also, I'm not really sure this program is actually in use any more - in my mind it is one candidate for getting booted, but that also is not in scope for this change. So, I'm going to drop this issue.

    2. Works for me. Thanks.

  3. 
      
jeffpc
  1. Ship It!
  2. 
      
richlowe
  1. 
      
  2. The loops here aren't, exactly, equivalent. Though the "old" side seems, at best, confused. I just wanted to point this out.

    1. Ok, lets try to unconfuse it:) At first, the for loop is declared with three parts:
      1. initialization - done once, before loop is entered;
      2. test or condition - evalueated and if true, then the loop is executed
      3. increment, which is executed after loop is done and before condition is evaluated.

      The old code:

      entry = mp->entries;
      for (; entry = find_matching_entry(entry, grubsign, grubroot, root_opt); entry = entry->next) { .... }
      

      In old code the initialization is not written as for statement, but before it. The condition statement is actually consisting of two actions, at first the find_matching_entry() is called and the result is assigned to variable entry, then the actual evaluation happens to check if entry is not NULL, so we can rewrite old condition as block { entry = find_matching_entry(entry, grubsign, grubroot, root_opt); entry != NULL }
      Fortunately the increment statement is trivial - entry = entry->next;

      For sake of simplicity, lets have the same for loop, but with empty body.

      For first iteration, following will happen:

      initialization: entry = mp->entries;
      condition statement 1: entry = find_matching_entry(entry, grubsign, grubroot, root_opt);
      condition statement 2: entry != NULL

      So we can rewrite the case of first iteration by using while():

      entry = find_matching_entry(mp->entries, grubsign, grubroot, root_opt);
      while (entry != NULL) {

      entry = entry->next;    /* increment */
      

      }

      Since our loop body is empty, we only have increment statement in the loop body.
      After for loop has evaluated the increment statement, the condition is evaluated again, in our case, the condition is composed of two statements, assignment and compare, so we need to update the while loop with assignment statement from condition:

      entry = find_matching_entry(mp->entries, grubsign, grubroot, root_opt);
      while (entry != NULL) {

      entry = entry->next;    /* increment */
      entry = find_matching_entry(entry, grubsign, grubroot, root_opt); /* first statement from condition */
      

      }

      For N'th iteration of while loop, the condition entry != NULL is evaluated, if TRUE, the loop is entered. As we still have empty loop, the only statements to be executed are increment statement and first statement from condition. With this, the loop body is processed and while statement will test condition entry != NULL.

      We can still rewrite the while loop above as:

      entry = find_matching_entry(mp->entries, grubsign, grubroot, root_opt);
      while (entry != NULL) {

      entry = find_matching_entry(entry->next, grubsign, grubroot, root_opt); /* increment + first statement from condition */
      

      }

      To make use of this rewrite of for loop in favor of while loop, the actual body of the former for loop has to be inserted before entry assignment statement, and if the continue statement is used, it has to be preceeded by increment and first statement of for loop condition.

      With this, the final while loop would be:
      entry = find_matching_entry(mp->entries, grubsign, grubroot, root_opt);
      while (entry != NULL) {
      {
      statements...
      if (...) {
      entry = find_matching_entry(entry->next, grubsign, grubroot, root_opt); / increment + first statement from condition /
      continue;
      }
      }
      entry = find_matching_entry(entry->next, grubsign, grubroot, root_opt); / increment + first statement from condition /
      }

      The for loop is generic way to write while loops, but can get really messy if initialization, condition and/or increment statements are too complex.

      Now lets get back to the actual code; the body of old for loop has two statements: first if statement, and followed by for statement. Since if statement is using continue in its body, in rewrite, we have to insert increment+first part of the condition before continue.

      For inner for loop, we dont have to do anything, as neither break nor continue in inner for, will not affect outer loop and goto is not used. And again, at the end of the outer loop, we need increment+first part of the condition.

  3. 
      
tsoome
richlowe
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...