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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
