Evan,
Looks okay.
-ethan
Evan Layton wrote:
> 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
>>>
>
