Hi Niall,
On 23/12/2011 04:13, Niall Power wrote:
> Hi Darren,
>
>
> Thanks for reviewing. I've responded to some of your points below - the
> AI target selection comments I will defer to Dave :)
>
>
> On 12/23/11 01:38 AM, Darren Kenny wrote:
>> Hi Niall,
>>
>> Working my way through some files, here are some comments:
>>
>> usr/src/Makefile.master
>>
>> - I would think that the DC and AI DTD versions should really be updated
>> here to reflect the fact that the targets version being imported in them
>> has changed too. The main reason being this would be the best way for a
>> user/consumer of the DTD (writing an XML manifest) to be specific about
>> the expected versions of things.
> Yeah, I think you have a point there alright. There would be no visible
> changes to DC manifests in practice because we only changed the physical
> section, and DC only uses the logical section I believe. But there
> certainly will be changes to the AI manifest for use of GPT (we will
> also need to update docs, man pages and sample manifests for this). I am
> not sure what is the appropriate action to take here, but I will
> certainly adjust the AI and DC DTD versions if you and others feel this
> is the appropriate thing to do.
Ok, if that's the case the DC manifest could definitely remain unchanged.
I've been thinking about this a little more, and it may be possible to
leave the AI DTD version unchanged too. I know I'm arguing against my self,
but it's not clear-cut.
The change made for GPT partitions is a compatible change - in that any
existing AI manifest will still validate against the new manifest.
Historically this would be a reason not to up the version of a library, but
this is not a library ;)
For AI, the main reasoning I had for changing the version, is that this is
the only publicly visible version of the DTD in any manifest - e.g. you
could end up taking a GPT based manifest and validating it against what
appears to be the same ai.dtd.1 but isn't, causing confusing validation
failures.
When it comes to the possibility of having software recognise different
versions of a manifest for capabilities, it would have a harder time
discerning the version of the manifest if we don't change the AI DTD version ;)
It would be good to have input from others on this one all right :)
>
>>
>> usr/src/cmd/auto-install/checkpoints/target_selection.py;
>>
>> - lines 1150-1154 and 1156-1160
>>
>> I'm fairly sure we could remove the need to do this condition check
>> twice. Possibly using a temporary variable to store the size.
>>
>> - lines 1805 to 1811
>>
>> Are these lines commented out intentionally? If so, why?
>>
>> - lines 2000-2003
>>
>> The error refers to slices - what terminology should we be using here?
>> Given that we have gpt_partitions in the DTD, seems wrong to to refer to
>> them as such in the errors too...
>>
>> usr/src/lib/install_common/__init__.py:
>>
>> - lines 478-490:
>>
>> Why aren't we using the built in os.sysconf() method to get the
>> pagesize, and number of pages, e.g.:
>>
>> $ python
>> >>> import os
>> >>> print os.sysconf('SC_PAGESIZE')
>> 4096
>> >>> print os.sysconf('SC_PHYS_PAGES')
>> 1957677
>
> Dave wrote this so I will ask for his take......
> Dave? :)
>
>
>> usr/src/lib/install_target/discovery.py:
>>
>> - lines 1104-1116:
>>
>> Why is it necessary to mount the ESP partition here, would fstyp not
>> give you the fs format?
>
> You probably saw Seth's response already. fstyp is unreliable.
> Have a look at 6913949 for further tails of woe.
Ok, on this basis I understand, but I think that it should be alluded to in
the comments somehow then so that is known to someone looking at this code
at a later date.
>
>>
>> usr/src/lib/install_target/logical.py:
>>
>> - lines 93-97, 100-104, 193-195, 511-513:
>>
>> This shouldn't be necessary and should be enforced by the DTD.
>>
>> But if you wish to have it here, it looks like this would be best done
>> as a common method rather than repeating the code like this.
>
> Dave did this however he didn't change any actual functionality but
> rather improved the logic of the implementation.
Sure, I can see that it works quite well, just would be cleaner as a
function I feel.
>
>>
>> I also see similar else where in other files like physical.py.
>>
>> usr/src/lib/install_target/shadow/__init__.py:
>>
>> - lines 124-126:
>>
>> These should probably be removed.
> Yep. Dave has removed these in the lastest slime_uefi source.
Ok.
Thanks,
Darren.
>
>
> Thanks!!
>
> Niall
>
>> Thanks,
>>
>> Darren.
>>
>> On 19/12/2011 09:19, Niall Power wrote:
>>> Hi!
>>>
>>> Myself, Drew and Dave Marker would like to ask for code reviewers for the
>>> Install UEFI, GRUB2 and large disk boot project.
>>>
>>> I've broken down the webrev into component groupings and specified people
>>> who we would particularly like to review particular components (eg. leads
>>> for those compononents) but we welcome and appreciate any and all review
>>> comments from others too of course.
>>>
>>> Some things to be aware of:
>>> This is about 95%, but not yet 100% complete.
>>> The core is complete and functional but there are a few odds and ends that
>>> need to be done still:
>>> - usbgen and usbcopy need to be ported to be GRUB2 and UEFI compatible. I
>>> am figuring out this with Seth. Will create a separate webrev.
>>> - installadm changes for GRUB2 and UEFI. Sue is working on this and will
>>> be handled in a separate review
>>> - Installation of UEFI system into an MBR partitioned disk. This is a
>>> minor amount of work that doesn't impact the existing code changes (it will
>>> essentially add a small amount more code). I'll get this included in time
>>> for round 2, otherwise create a separate webrev.
>>>
>>> I'd like to set an initial timeout for collection of round 1 review
>>> comments on Friday Dec 23rd.
>>>
>>> Following that I propose to ring in the new year with a second round of
>>> review comments from Monday Jan 2nd until Friday Jan 6th.
>>>
>>> Please let me know if you intend to review any components, or if you are
>>> unavailable to review specific requested components.
>>>
>>> *Webrev URL:
>>> ----------------*
>>> https://cr.opensolaris.org/action/browse/caiman/niall/slim_uefi_version_01/webrev-uefi-ver-1/
>>>
>>> Most of the files below have only a very small number of lines of code
>>> changes so it's not as big a task as it looks.
>>>
>>> *
>>> Makefile and Misc.: Volunteers
>>> ----------------------------------------*
>>> usr/src/Makefile.master
>>> usr/src/Targetdirs*
>>> *usr/src/lib/Makefile.targ
>>> usr/src/lib/install_common/__init__.py.src
>>> *
>>> *
>>> *Targets: Dermot, Darren, Matt
>>> ---------------------------------------*
>>> usr/src/pkg/manifests/system-library-install.mf
>>> usr/src/lib/install_manifest/dtd/target.dtd
>>> usr/src/lib/install_target/Makefile
>>> usr/src/lib/install_target/__init__.py
>>> usr/src/lib/install_target/controller.py
>>> usr/src/lib/install_target/cuuid.py
>>> usr/src/lib/install_target/discovery.py
>>> usr/src/lib/install_target/instantiation.py
>>> usr/src/lib/install_target/libdiskmgt/diskmgt.py
>>> usr/src/lib/install_target/libefi/Makefile
>>> usr/src/lib/install_target/libefi/__init__.py
>>> usr/src/lib/install_target/libefi/cfunc.py
>>> usr/src/lib/install_target/libefi/const.py
>>> usr/src/lib/install_target/libefi/cstruct.py
>>> usr/src/lib/install_target/libefi/efi.py
>>> usr/src/lib/install_target/logical.py
>>> usr/src/lib/install_target/physical.py
>>> usr/src/lib/install_target/shadow/__init__.py
>>> usr/src/lib/install_target/shadow/physical.py
>>> usr/src/lib/install_target/test/test_shadow_list.py
>>>
>>> *AI: Darren, Matt
>>> --------------------*
>>> usr/src/cmd/auto-install/checkpoints/target_selection.py
>>> usr/src/cmd/auto-install/test/test_target_selection_gpt.py
>>>
>>>
>>> *DC and Boot: Ethan, Seth, Drew
>>> -----------------------------------------*
>>> usr/src/cmd/distro_const/checkpoints/create_iso.py**
>>> usr/src/lib/install_boot/boot.py
>>> usr/src/lib/install_boot/test/test_boot.py
>>> usr/src/tools/tests/tests.nose
>>> *
>>> GUI: Darren, Dermot
>>> ---------------------------*
>>> *
>>> *usr/src/cmd/gui-install/src/Makefile
>>> usr/src/cmd/gui-install/src/confirm_screen.py
>>> usr/src/cmd/gui-install/src/disk_panel.py
>>> usr/src/cmd/gui-install/src/disk_screen.py
>>> usr/src/cmd/gui-install/src/fdisk_panel.py
>>> usr/src/cmd/gui-install/src/gpt_panel.py
>>> usr/src/cmd/gui-install/src/screen_manager.py
>>> usr/src/cmd/gui-install/xml/Makefile*
>>> *usr/src/pkg/manifests/system-install-gui-install.mf *
>>> *
>>> *
>>> I seriously wouldn't bother code reviewing these glade auto-generated XML
>>> files but if you enjoy pulling your eyes out :)*
>>> usr/src/cmd/gui-install/xml/fdisk-panel.xml
>>> usr/src/cmd/gui-install/xml/gpt-panel.xml
>>> usr/src/cmd/gui-install/xml/installationdisk.xml
>>> *
>>> **
>>> Text Install: Karen + Volunteer
>>> --------------------------------------*
>>> usr/src/cmd/text-install/Makefile
>>> usr/src/cmd/text-install/__init__.py
>>> usr/src/cmd/text-install/disk_selection.py
>>> usr/src/cmd/text-install/disk_window.py
>>> usr/src/cmd/text-install/fdisk_partitions.py
>>> usr/src/cmd/text-install/gpt_partitions.py
>>> usr/src/cmd/text-install/helpfiles/Makefile
>>> usr/src/cmd/text-install/helpfiles/gpt_partitions.txt
>>> usr/src/cmd/text-install/partition_edit_screen.py
>>> usr/src/cmd/text-install/progress.py
>>> usr/src/cmd/text-install/summary.py
>>> usr/src/cmd/text-install/ti_install.py
>>> usr/src/cmd/text-install/ti_install_utils.py
>>> usr/src/cmd/text-install/ti_target_utils.py
>>> usr/src/cmd/text-install/welcome.py*
>>> *usr/src/pkg/manifests/system-install-text-install.mf
>>> *
>>> *Thanks!!
>>>
>>> Niall, Dave& Drew
>>>
>>> *
>>> **
>>> *
>>>
>>>
>>> _______________________________________________
>>> 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
