Alok Aggarwal wrote:
> Hi Ethan,
>
> Thanks for the review.
>
> On Thu, 17 Sep 2009, Ethan Quach wrote:
>
>
>> usr/src/cmd/Makefile.cmd
>> ------------------------
>> 45 - nit - Why not just call this ROOTLIBSVCSHARE ?
>
> Is there precedent to call it ROOTLIBSVCSHARE instead?
Just that some of the other ROOT*** variables are named as
their explicit path values. e.g. /lib/svc/method is
ROOTLIBSVCMETHOD
>> usr/src/cmd/auto-install/svc/manifest-locator
>> ---------------------------------------------
>> 78-82, 102-104 - This should be done in get_manifest itself.
>
> I don't think get_manifest should make the assumption
> that it can get called just on the console.
That's fair enough then. Though I have a couple of
additional questions then ...
1)
TERM=sun-color
export TERM
Does this work properly for all cases, even when a user
is tipped in via serial line?
2)
exec </dev/console >/dev/console 2>&1
Is this a workaround for some known issue? If so, do you
know what?
> In this
> specific case, the caller knows that it will be called in a
> certain scenario so it seems that the caller should do
> the appropriate setup prior to calling get_manifest.
>
>> 57-60 - Can we loop and process instead of processing a set
>> number of args?
>
> Sure, I can look into that.
>
>> 70 - I'd rather see this code be a bit more generic across
>> boot style here, but I guess that's going to have to wait
>> until we normalize our args for the 'net' boot case with
>> media boot.
>
> I do think that we need to normalize our args for the
> 'net' case in order to address accidental reboot in AI.
>
>> 76,77 - So this means that 'install' and 'prompt' are
>> positional then. Did we consider this when we originally
>> chose these paramenters?
>
> Yep.
>
>> 107 - For x86, there's no equivalent for booting into
>> the media without starting auto-install then? If one were
>> to edit a grub menu entry and remove aimanifest=, it'd
>> seem we'd just try to do service_discovery and die?
>
> Good catch, that's a bug. I've changed it so that if it's
> aimanifest=, the auto-installer service will be disabled
> and manifest-locator will exit with SMF_EXIT_OK. And, drop
> you into a shell.
I wasn't quite sure if that was the correct course of action
at this point, but if you think that's the right thing to do now
and doesn't break any other usage cases, then okay.
>
>> usr/src/cmd/auto-install/svc/manifest-locator.xml
>> -------------------------------------------------
>> 22 - 2008 -> 2009
>>
>> Should manifest-locator also depend on some sort of
>> networking?
>
> manifest-locator depends upon filesystem/minimal and
> through the dependency chain ends up depending upon
> system/identity:node and indirectly network/physical.
I won't block on this, but I'd much prefer an explicit
dependency declaration if we know this service potentially
needs to wait for networking (i.e. for the service discovery
case)
>
>> usr/src/cmd/install-tools/Makefile
>> ----------------------------------
>> 37,39,50,57 - nit - can you tab the values over one more to be
>> consistent with the rest of the file?
>
> All the changes to this file will go away as part
> of deleting getbootargs.
okay
>> usr/src/cmd/slim-install/svc/live-root-fs-minimal.xml
>> -----------------------------------------------------
>> 87 - Is there any other way of making this determination
>> for x86?
>
> I don't think so, our code uses the same check all over
> the place.
The code is in other places, but it seems this is the first
place we're using it to determine whether or not we're booted
from media or from net.
My concern is that we're using an implementation detail to
imply meaning of something that's not necessarily related.
What happens if a user edited the grub menu entry when
booted to AI bootable media and added 'install_media' ?
(I think we want this case to actually work eventually,
to support users who need to use the bootable media as a
boot-shim to get around DHCP or other network limitations.)
If parsing the boot args is our only means of differentiating
right now, could we just define a new boot arg to determine
this explicitly? e.g. media_boot=true or something?
thanks,
-ethan
>
> Thanks,
> Alok
