Hi,

there is a tarball of patches against individual files attached to this email. This set of patches adds new driverdisc format support as well as the automatic driverdisc pickup (known as dlabel in RHEL5) to the current master branch.

Internally this is supposed to work like this:

- the new format is documented in the docs/ directory

- when unpacking
**- whole RPMs go to /tmp/DD-number/
**- firmware files go to /tmp/DD/lib/firmware (in overwriting mode)
**- modules go to /tmp/DD/lib/modules (in overwriting mode)

- during DD loading
**- depmod and modprobe are set to prefer the /tmp/DD location
**- when modprobe runs in DD mode, it picks modules from /tmp/DD, the only exceptions are modules needed to satisfy dependencies
**- probeDevices (udev trigger) is started after each driver disc (with modprobe in DD mode)

- at the end, modprobe is set to preference mode only and probeDevices is ran again

- during Yum install phase
**- the code lists all modules loaded into kernel and tried to find those residing in DD directory (/tmp/DD)
**- modules which were loaded from that directory are translated to package names (file dependencies) and added to Yum transaction

Overall, this code should ensure all features from rhel5 are present in rhel6. It also adds the multiple driverdiscs capability. some issues are still to be solved (e.g. updating specific modules, which are needed for the DD to function).

There was also proposal for automatic updates.img pickup from driverdiscs, but there two main issues remaining:
- versioning, we do not want to be updating anaconda files with older version
- arch specific stuff, especially isys

I'm waiting for your comments. Just be aware of the fact that this must be present in rhel6, so I'm interested only in "how to improve" messages over messages complaining about the feature being unnecessary/useless.

--
Martin Sivak
Anaconda team / Brno, CZ
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On Tue, 2009-12-01 at 07:03 -0500, Martin Sivak wrote:

> there is a tarball of patches against individual files attached to this
> email. This set of patches adds new driverdisc format support as well as
> the automatic driverdisc pickup (known as dlabel in RHEL5) to the current
> master branch.

Thanks Martin. I am happy to see this. I hope that version 3 is the last
major change that we need to make to driver disk support

> - during DD loading
> - depmod and modprobe are set to prefer the /tmp/DD location

(note: we added support for additional search paths to do this, so
depmod is temporarily told to include and prefer an external location,
then it isn't. This lets us load everything via regular udev too, which
is also known as doing it the right way).

> - during Yum install phase
> - the code lists all modules loaded into kernel and tried to find
> those residing in DD directory (/tmp/DD)
> - modules which were loaded from that directory are translated to
> package names (file dependencies) and added to Yum transaction

We talked about that earlier - is this already the modinfo stuff? Cool.

Do you have an initrd image anywhere that Alex He or someone else in QE
can get at it? I can build one, but I'm a little short on time at the
moment so that would speed up a quick test run.

Jon.


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hi,

> > - during Yum install phase
> > - the code lists all modules loaded into kernel and tried to find
> > those residing in DD directory (/tmp/DD)
> > - modules which were loaded from that directory are translated to
> > package names (file dependencies) and added to Yum transaction
>
> We talked about that earlier - is this already the modinfo stuff?
> Cool.

It is, it wasn't particulary hard to implement. I don't really like the modinfo calls, but since there is no library for this, I had no other choice.

> Do you have an initrd image anywhere that Alex He or someone else in
> QE
> can get at it? I can build one, but I'm a little short on time at the
> moment so that would speed up a quick test run.

Not at the moment, rawhide doesn't even start anaconda properly (last time I checked - yesterday) so I couldn't test anything myself..

Martin

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

> > Do you have an initrd image anywhere that Alex He or someone else in
> > QE
> > can get at it? I can build one, but I'm a little short on time at
> the
> > moment so that would speed up a quick test run.
>
> Not at the moment, rawhide doesn't even start anaconda properly (last
> time I checked - yesterday) so I couldn't test anything myself..

Also, there are some changes to stage2 which needs to be there for the packages to install, so I would like to merge it into the main tree as soon as possible, to get nightlies with it.

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On Tue, 2009-12-01 at 09:23 -0500, Martin Sivak wrote:
> > > Do you have an initrd image anywhere that Alex He or someone else in
> > > QE
> > > can get at it? I can build one, but I'm a little short on time at
> > the
> > > moment so that would speed up a quick test run.
> >
> > Not at the moment, rawhide doesn't even start anaconda properly (last
> > time I checked - yesterday) so I couldn't test anything myself..
>
> Also, there are some changes to stage2 which needs to be there for the packages to install, so I would like to merge it into the main tree as soon as possible, to get nightlies with it.

Yes, I would really like that. Ok. So I'll keep a look out for this to
hit the tree - I don't see why it shouldn't go in, but not my call

Jon.


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On 12/01/2009 01:03 PM, Martin Sivak wrote:

Hi,

there is a tarball of patches against individual files attached to this email. This set of patches adds new driverdisc format support as well as the automatic driverdisc pickup (known as dlabel in RHEL5) to the current master branch.



Hi,

Generally: did you test this? Does the C code build? Does the python
code run when you use it to update the f12 code? (the current master does)


> diff --git a/docs/driverdisc.txt b/docs/driverdisc.txt
typorior the DriverDisc extraction

> diff --git a/loader/cpio.h b/loader/cpio.h
> diff --git a/loader/cpio.c b/loader/cpio.c
My viewer shows some tab characters in the new code, for instance in the
files above, but others as well.


> diff --git a/loader/driverdisk.c b/loader/driverdisk.c
> /* FIXME: depmod didn't run */
Perhaps just return some error code.
How about all those other error handle todos and fixmes?
The functions that are passed into explodeRPM() are going to dump text
directly to stdout. Shouldn't that go into a log instead?


>diff --git a/loader/rpmextract.c b/loader/rpmextract.c
The explodeRPM() function is around 200 lines.

> diff --git a/backend.py b/backend.py
>+ for f in glob.glob("/tmp/DD/firmware/*"):
Guessing here, but shouldn't this be "/tmp/DD/lib/firmware/*" ?

Ales

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tue, 1 Dec 2009, Martin Sivak wrote:


Hi,

there is a tarball of patches against individual files attached to this email. This set of patches adds new driverdisc format support as well as the automatic driverdisc pickup (known as dlabel in RHEL5) to the current master branch.

Internally this is supposed to work like this:

- the new format is documented in the docs/ directory

- when unpacking
**- whole RPMs go to /tmp/DD-number/
**- firmware files go to /tmp/DD/lib/firmware (in overwriting mode)
**- modules go to /tmp/DD/lib/modules (in overwriting mode)

- during DD loading
**- depmod and modprobe are set to prefer the /tmp/DD location
**- when modprobe runs in DD mode, it picks modules from /tmp/DD, the only exceptions are modules needed to satisfy dependencies
**- probeDevices (udev trigger) is started after each driver disc (with modprobe in DD mode)

- at the end, modprobe is set to preference mode only and probeDevices is ran again

- during Yum install phase
**- the code lists all modules loaded into kernel and tried to find those residing in DD directory (/tmp/DD)
**- modules which were loaded from that directory are translated to package names (file dependencies) and added to Yum transaction

Overall, this code should ensure all features from rhel5 are present in rhel6. It also adds the multiple driverdiscs capability. some issues are still to be solved (e.g. updating specific modules, which are needed for the DD to function).

There was also proposal for automatic updates.img pickup from driverdiscs, but there two main issues remaining:
- versioning, we do not want to be updating anaconda files with older version
- arch specific stuff, especially isys

I'm waiting for your comments. Just be aware of the fact that this must be present in rhel6, so I'm interested only in "how to improve" messages over messages complaining about the feature being unnecessary/useless.



I would like to review it, but could you send it as a series of patches by
task rather than by file? It would make it easier to follow and comment on.

First glance through the code, here are issues I have:

1) Inconsistent Python style. Indentation, use of tabs, etc. I would comment
on specific examples if patches were emailed to the list.

2) The driverdisk.txt file needs to be formatted correctly for an 80 column
terminal.

3) Inconsistent style in code introduced in loader. If you're going to use
tabs, do that. If you're going to use spaces, do that. But don't mix the
two. Personally, I would prefer spaces for everything and we don't have to
work with tabstops. Anaconda has been moving in the direction of spaces in
the code, not tabs.

4) A lot of code added to the loader that I question the value of, plus wonder
what sort of difficulties we will run in to later on during RHEL-6's lifespan.
The cpio code for instance. Why? Could we just include the cpio program and
exec that from loader? If we must have code in the loader, why not use
libarchive?

5) The rpm extraction code introduced is also worrysome. If it's based on
rpm2cpio, could we just include that program in the initrd and use it directly
rather than maintaining our own rpm extraction code?

6) The modulesWithPaths() function introduced in isys assumes a lot and
handles no exceptions. The modPaths assignment line is difficult to read.

7) The /tmp/DD/... paths throughout the code would probably be better as a
constant defined once and lengths used in the code changed to compute at
runtime rather than hardcoded.

That was a quick run through. I'd like to comment further, but I'll wait for
you to send task-based patches to the list.

- --
David Cantrell <dcantrell@redhat.com>

Red Hat / Honolulu, HI

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iEYEARECAAYFAksVbBEACgkQ5hsjjIy1Vkm3kgCguOmjxLThPM ZBsVKzeJDs091O
4kwAoLs+N4mYUn57CtiQoymWleRMR6tM
=kl8W
-----END PGP SIGNATURE-----_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

Hi,

Sorry no task based patches this time.

I needed to post it this way as partners want to apply it and test it. I can give you access to the git repo, so you can see the patch flow, but managing twenty (or so) email threads + all replies is not really comfortable... Especially when there is a lot of backporting, prototyping and stub filling going on in the source.

We also should probably refine our patch review process a bit, because it is generally hard to keep track of unreviewed patches.. and big changes are even worse to keep an eye on. Just the number of replies is not sufficient info about the result of review process unfortunately.

> 1) Inconsistent Python style.
> 2) The driverdisk.txt file needs to be formatted correctly
> 3) Inconsistent style in code introduced in loader.

Coding style.. well that is easy to fix.

The python code I touched shouldn't have any new tabs as I have expandtabs on in my vimrc. The backported code is quite different story though..

I have finally changed expandtabs settings for my C files too, so this shouldn't happen anymore.

> 4) A lot of code added to the loader that I question the value of,
> plus wonder
> what sort of difficulties we will run in to later on during RHEL-6's
> lifespan.
> The cpio code for instance. Why? Could we just include the cpio
> program and
> exec that from loader? If we must have code in the loader, why not
> use
> libarchive?

Never heard of libarchive and when I was looking for a library which could do what I wanted, even the RPM people couldn't tell me any hints for cpio unpacking (they have their own). Using external library would be easier, but the cpio routines in this patch are really simple and work directly with the RPMs. Using libarchive would mean dumping the data somewhere and then calling the library.

Unfortunalely RPM itself doesn'ŧ expose this API and is missing the filename/size filter callbacks. libarchive looks interesting and should support everything we need, but it will cost us memory because of uncompressed data duplication.

And because 16th is quite close now, can you live with this code staying until we get the feature working in rhel6? It exposes only one method with couple of callbacks and we can always replace it later.

> 5) The rpm extraction code introduced is also worrysome. If it's
> based on
> rpm2cpio, could we just include that program in the initrd and use it
> directly
> rather than maintaining our own rpm extraction code?

I have to check the provides section to select the right RPMs to extract, so there is no way I can avoid calling librpm or rpm binary itself (I prefer the library..). The code uses only official RPM API and has no hardcoded payload types.

> 6) The modulesWithPaths() function introduced in isys assumes a lot
> and
> handles no exceptions. The modPaths assignment line is difficult to
> read.

It assumes /proc/modules has a module name in it's first column and modinfo returns text lines. This is standard API and hasn't changed for ages (expecially when we are talking about rhel). If we're out of memory or are missing /proc, we are screwed anyways. So I think that exception handling here will just move the error somewhere else and I prefer to see the exact place of traceback. And errors which could cause traceback here are only really serious kernel changes or not enough memory, don't you think?

List comprehension with one for and one if hard to read? I could use for/if/append approach, but I found this much more elegant (also this is the python way, look what they did to map/reduce/filter functions). This one is also easy to rewrite if you wish so though.

> 7) The /tmp/DD/... paths throughout the code would probably be better
> as a
> constant defined once and lengths used in the code changed to compute
> at
> runtime rather than hardcoded.

True, but the /tmp/DD-* paths were always this way (which doesn't mean it is right and I will change it).

Martin

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list

On Wed, Dec 2, 2009 at 8:38 AM, Martin Sivak <msivak@redhat.com> wrote:
> Sorry no task based patches this time.
>
> I needed to post it this way as partners want to apply it and test it. I can give you access to the git repo, so you can see the patch flow, but managing twenty (or so) email threads + all replies is not really comfortable... Especially when there is a lot of backporting, prototyping and stub filling going on in the source.

Task based patches are the *only* way to be able to sanely review and
integrate patches. If someone randomly came and dropped a patch like
this on the list, we'd tell them to please reformat their patches and
send them correctly. Why do you for some reason get a pass on that?
Deadlines are not a compelling reason; if anything, a deadline makes
it even more important to get them sorted out and posted in an easy to
review format as there's less room for errors.

- Jeremy

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list