Ethan Quach wrote:
> Evan,
>
> Just a couple of nits now ...
>
> 386-390 - Why not just assign the entry from tmp_entries[] to entries[]
> instead allocating a new copy of the string, and just freeing the old?
> i.e.
>
> entries[num_lines++] = tmp_entries[i];
> tmp_entries[i] = NULL;
fixed
>
>
> 414 - To prevent dup'ing and free'ing every other line for entries you
> don't
> care about, the if() condition can be:
>
> } else if (found_title && !found_orig_be) {
>
fixed
Thanks for the comments, it looks a lot cleaner now! :-)
-evan
>
> thanks,
> -ethan
>
>
> Evan Layton wrote:
>> 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
>>>>
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
