Ethan Quach wrote: > Evan, > > 355 - Shouldn't this be set to 0 ?
Yes it should. In fact with it set to one if the orig BE is the first entry we end up with no title line... > > 386 - Instead of setting num_lines = num_tmp_lines here, initialize it > to 0, > and use (and increment) it as the index to entries[] at lines 392 and 399. I added the use of num_lines and incremented it as suggested. > > 431, 451 - If entering these if() clauses, you need to check if entries[] > if populated, and if so, free it. (I would suggest setting a ret code, > and then > jumping down to a cleanup section below.) I took another look at it and I was doing that clean-up when we found a duplicate entry for the new BE. I've moved that down to a clean-up and return section similar to what you suggested. I've retested and updated the webrev. -evan > > > thanks, > -ethan > > > Evan Layton wrote: >> Hi Ethan, >> >> I've corrected these two issues, retested and updated the webrev. The >> code now does the collection of the menu entry lines at the point we >> detect that we've found the original BE's entry. Once we have that >> information we continue through the file making sure we don't also >> have a duplicate entry. Finally we collect the menu entry lline and >> maintain their order so that the new BE's menu entry will also match >> the order of the entry we're copied from. >> >> Thanks! >> -evan >> >> Ethan Quach wrote: >> >>> Evan, >>> >>> The existing code accounts for the case where if the new BE we're adding >>> already has an entry in the menu.lst, it does the proper thing. >>> However, >>> with your changes, it might fail to detect that. For the case where >>> the new >>> BE already has an entry but is below orig_root_ds entry, you no longer >>> find it. >>> >>> >>> Is it possible for a line to exist be between the 'title' line and >>> 'bootfs'? >>> If so, that line would have been recorded in entries[] and written out >>> after the 'bootfs' line for the new BE. I don't know if that would >>> cause >>> a problem but just something to note. >>> >>> >>> thanks, >>> -ethan >>> >>> >>> Evan Layton wrote: >>> >>>> I need to get a review the changes for >>>> >>>> 3654 libbe should keep menu.lst customizations when cloning >>>> http://defect.opensolaris.org/bz/show_bug.cgi?id=3654 >>>> >>>> The webrev is available from: >>>> http://cr.opensolaris.org/~evanl/webrev.3654/ >>>> >>>> This fix changes the way we build grub menu entries. When we're >>>> creating a new BE we take a look at the grub entry lines from the >>>> parent of BE we're creating and we edit the title and bootfs lines >>>> the rest of the lines are copied directly form the parents entry. >>>> >>>> This has been tested using "beadm create" and "beadm activate" which >>>> call into be_copy() and be_activate() which are the only consumers >>>> of this functionality. I've also verified that changed entries in >>>> menu.lst are copied from the parent BE to the entry for the >>>> clone/copy. >>>> >>>> These changes also include some simple lint clean-up. >>>> >>>> Thanks, >>>> -evan >>>> _______________________________________________ >>>> caiman-discuss mailing list >>>> caiman-discuss at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>
