Sorry to chime in a little late. I wasn't following the issue tracker..
Anyway, I wanted to comment on Jeremy's comments about the style of the
coding.
In order to facilitate other's understanding, I'm copying and pasting
jeremy's comments..
>Hrmmm, I'm not sure that I like the approach being taken here.
Before getting into
> details, a couple of process related things:
>* Having the whole thing as a unified diff makes things far simpler to
review. The simple >way to do this is to copy files before you modify
them to file.foo and then run `gendiff >anaconda .foo`. For new files,
touch newfile.foo and then it'll get included inthe diff as >well.
>* Instead of copying and pasting functions and commenting old versions
out, please just put >your changes inline. It makes the diff much more
difficult to parse otherwise.
Your points well taken. I'll stick to them from now on..
>1) Stylistically, this doesn't fit in properly with anaconda. We don't
pass around state
>via environmental variables, instead, the flags structure is used with
all of the defines
>in loader.h. Then, parsing of /proc/cmdline can occur in one place
> (loader.c:parseCmdLineFlags())
One of the reasons ,I used environment variables is to access them in
consistent manner between loader and anaconda. Otherwise I would have to
touch more codes to pass them to anaconda in some way, e.g. as command
line arguments..
>2) Doing replacements of things inside the kickstart config is going
to be fragile. I'd
>really prefer to avoid that. Two possible approaches for doing so
would be
> a) Decide to use something like biosdisk0 -> biosdisk7 to correspond
to bios disks 0x80
>-0x87 and just then use that for corresponding to the disk and then use
biosdisk0p1 for the >first partition. (biosdisk or bd or something that
doesn't actually exist, not completely >sold on which syntax)
> b) Instead of using the existing directives, add a new one of bd:
instead of places
>where hd: gets used for defining a hard drive. Then, --onbiosdisk or
similar with
>partitioning commands. This is probably the better approach, but I
haven't thought through >it as completely
My idea here was to decouple the "EDD" stuff as much as possible from
the rest of the code.This would allow
development/modification/experimentation with "EDD" code independently
from the rest of the bulk. With this in mind, to parse boot options or
ks.cfg, I introduced some hooks into the loader/anaconda code
(parseEDDHDCommand,EDDfy etc.)that would behave as wrapper/adapter. They
allow "current" options to pass through without any scrutiny and will
convert any new "EDD options" into their corresponding "current"
format.The back end, which makes sense of this options remains the
same...
>Why have the noedd flag -- edd will only get utilized if people request
it, especially if
>we're not rewriting the ks.cfg. Does loading the module cause an oops
in some cases?
I used it basically to do two things 1) load the edd.o module 2)
activate hooks in loader/anaconda that deals with EDD specific options..
>5) regexp are likely to increase the size of the loader greatly -- are
they really needed?
Otherwise, I probably needed to write a tiny state machine. I
went for the easier way :-)
>6) Parsing of /proc/partitions isn't recommended (your current parsing
will miss cciss and >other Weird (tm) devices for example). Instead,
getting a list of drives from kudzu is
> better as that's then completely consistent
Point well taken.. I'll see if I can use kudzu instead...
>7) With the way you're currently doing things, you're not guaranteed to
have the device
> node when you need it -- we don't create the full set of devices in
/dev until we get to
> the second stage and the loader always makes device nodes in /tmp as
needed using
> devMakeInode() from isys
Aahah.. if you look into the readDiskSig/writeDiskSig in eddsupport.c
you'll see that I'm indeed using the devMakeInode interface..
I did some preliminary testing before sending you the code..So the
code works at least with some simple configs...
Thanks..
--rez
> -----Original Message-----
> From: anaconda-devel-list-bounces@xxxxxxxxxx
> [mailto:anaconda-devel-list-bounces@xxxxxxxxxx]On Behalf Of
> Jeremy Katz
> Sent: Friday, June 25, 2004 9:54 AM
> To: Domsch, Matt
> Cc: anaconda-devel-list@xxxxxxxxxx
> Subject: Re: EDD disksigs for disks other than 0x80
>
>
> On Thu, 2004-06-24 at 15:40 -0500, Matt Domsch wrote:
> > Patch below against 2.6.7-bk(current) gives mbr_signature for first
> > six BIOS disks.
>
> Seems to work, at least for two (all that I have handy on a
> test machine
> right now). Unfortunately, the mbr_signature on both of those disks
> (and the sig on the first from before rebooting into the newer kernel)
> is 0x00000000. So I guess these don't have any guarantee of
> uniqueness?
>
> Jeremy
>
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
>
