Hi Drew,
Looking more like it :)
On 27/05/2011 16:23, Dermot McCluskey wrote:
> Drew,
>
> This looks fine to me.
>
>
> Just one further nit: this line seems a bit strange:
> install_target/physical.py:
>
> 837 if size != LOGICAL_ADJUSTMENT and size > LOGICAL_ADJUSTMENT:
>
> I presume this should just be:
>
> 837 if size > LOGICAL_ADJUSTMENT:
Is that not more:
if size >= LOGICAL_ADJUSTMENT ?
>
> Also, that block of code re-uses the variable "size" to be
> both a long and a Size object, which might be confusing?
> I'd suggest renaming the 2nd size to, eg size_obj or gap_size?
>
> Neither of these nits require an updated webrev.
>
I agree,
Thanks,
Darren.
>
> - Dermot
>
>
>
> On 05/27/11 15:53, Drew Fisher wrote:
>> Ok, round 3.
>>
>> http://cr.opensolaris.org/~drewfish/cr_7048167/
>>
>> I took into consideration all comments from round 2, including the
>> import nits. I also updated both the get_logical_partition_gaps() and
>> shadow/physical.py to decrease the size of the logical partition IF we
>> change the start sector.
>>
>> Thanks for looking!
>>
>> -Drew
>>
>> On 5/27/11 5:23 AM, Darren Kenny wrote:
>>> Drew,
>>>
>>> Much closer to where we want to be I think...
>>>
>>> On 27/05/2011 11:50, Dermot McCluskey wrote:
>>>> Drew,
>>>>
>>>> One question and a few nits:
>>>>
>>>>
>>>> Question:
>>>> When ensuring that a new logical is at least 63 sectors from other
>>>> logicals, you
>>>> restrict
>>>> the check to newly created partitions:
>>>>
>>>> 356 # verify there are not more than MAX_EXT_PARTS
>>>> 357 logical_list = [p for p in self._shadow if
>>>> p.is_logical and \
>>>> 358 p.action == "create"]
>>>>
>>>> Should "preserve"d partitions (or even "use_existing_solaris2" ones)
>>>> not also be
>>>> checked?
>>> I agree, this should be p.action != "delete".
>>>
>>> But also, we should not adjust any existing partitions later in this
>>> code block,
>>> so before adjustment is made, we should check if p.action == "create".
>>>
>>>>
>>>> Nits:
>>>> - install_target/physical.py should import LOGICAL_ADJUSTMENT from
>>>> shadow.physical rather than redefine itself
>>>> - test_shadow_list.py should also import LOGICAL_ADJUSTMENT from
>>>> shadow.physical and replace about 6 hard-coded references to "63".
>>>> (No need to re-send webrev if you decide to fix these nits.)
>>>>
>>> Some more comments on top of this:
>>>
>>> - In install_target/phyiscal.py where we're getting gaps:
>>>
>>> - Should we calculate the gaps to be the size we want here i.e.
>>> the gap minus
>>> 63, that way no adjustment should be necessary later if this is
>>> used.
>>>
>>> - In checking if there is a hole, should we return any hole< 63
>>> rather than
>>> exactly 63? A gap of< 63 in an extended partition really isn't
>>> going to be
>>> of any use for anything.
>>>
>>> Thanks,
>>>
>>> Darren.
>>>
>>>
>>>
>> _______________________________________________
>> caiman-discuss mailing list
>> [email protected]
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
