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;
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) {
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
>
