Hi Jeremy, sorry for taking so long to respond. I was out of the office for a while. First of all, thanks a lot for your thorough review. On 12/07/2008 04:56 PM, Jeremy Katz wrote: > On Dec 5, 2008, at 4:00 PM, David Cantrell wrote: >> If I don't hear anything from people after a few days, I will take it >> that people are ok with this patch going in to rawhide. That would be great, David. >> And a one line >> patch to mk-images to make sure the new helper script is installed. Maybe we also need to change a few more little things in the image building process to account for?: # prerequisites of this script to run inside RHEL5.2 installer initrd: # - copy udevsettle and udevd to initrd # - create /etc/udev/udev.conf with at least one comment line as content > So, some comments from trying to go through and at least look for > obvious problems/things to fix. Some of these may only be rawhide > applicable and given that the patch was against the RHEL5 codebase, they > may not be (easily) doable there > > * A lot of checking for things like a 2.6 kernel. There's _always_ > going to be at least a 2.6 kernel and such checks just needlessly > complicate the script, making it that much harder to see what's really > going on I suspect you mean the two places in linuxrc where we check for at least Linux 2.6.26 which is when IPv6 support for HiperSockets in layer 3 mode went upstream. In other words this is not about being a 2.6 kernel (we already assume that anyway), it is about a certain sub-version. The reason why I put this check in was to make my code base as independent as possible from a specific RHEL release. In fact I put the checks in hoping that (almost) no changes would be necessary for running under RHEL6 as opposed to my development environment under RHEL5.2. Lsznet raw was developed as a distro-independent tool which should work on a multitude of releases. Therefore, we check for some basic requirements. Does that make sense? > * Are udev and friends getting installed into the s390 initrd right now? Just a minimum base consisting of udevd and udevsettle just for calling the latter as we potentially dynamically "add" (potentially lots of, i.e. thousands of) devices by dynamically freeing them from the device ignore list cio_ignore. Since this may take quite some time we do not know upfront, we decided to use udevsettle instead of a bogus constant sleep hoping all devices would have been sensed until we wake up and continue and try to use them. There is really not more than udevd and udevsettle, there are no config file (at least no content) and no rule files. > * Some of the stuff in lsznet makes me think that modules should be > getting auto-loaded by udev given the existence of modaliases and > matching against things in /sys/bus/ccw Lsznet does not load any kernel modules at all. It relies on the caller to setup the environment and does auto-sensing based on the given environment. In case you were referring to the arrays related to CU type/model: They are there for mapping the values in sysfs to user-friendly strings that appear in the generated table of possible network hardware configurations. Since they contain more information than what is in modalias, alas, they cannot all be automatically extracted from the kernel modules. E.g. cardtype, devname, and groupchannels cannot be deduced automatically. With regard to linuxrc.s390, the way kernel modules are treated -- namely explicitly loaded including dependencies -- has been taken from the existing linxurc.s390 in order to introduce as few as possible changes. Also, trying to switch to auto-loading using udev would at least for the network types LCS and CTC which are based on cu3088 not be straight forward. The cu3088 module contains the modaliases but the corresponding driver modules (lcs/ctc) need to be loaded as well, see, e.g. RHIT 116546. > * Didn't we stop allowing telnetd and switch to only sshd for logging > into the install environment on s390 or am I just imagining things that > I wish we had done? :) Hm, telnetd does not get started. However, the strings that I took from the old linuxrc.s390 still mention telnetd. I'm open to fix this by removing the occurences of telnetd in the strings, if someone more knowledgeable than me confirms that telnetd really has been abandoned from the s390 installer initrd. > * Manual mknods shouldn't ever need to be present; udev should be > creating device nodes I would fully agree, if we used a full-fledged udev environment. But as stated above it is only there to wait for device sensing. I even start udevd very late, when (according to a running udevmonitor in my test environment) all udev events concerning non-s390-devices have already vanished. As for now, this code is just what has been in linuxrc.s390 before we touched it. Introducing full udev just for linuxrc.s390 just did not seem worth it. What do you think? > * modprobe instead of insmod if modules have to be manually loaded so > that at least dependent modules can be auto-loaded. This also lets us > kill the $LO hack I don't know why this $LO thing is there, I just kept it from the existing linuxrc.s390. In fact, insmod and friends come from busybox which is why it finds, extracts, and loads the modules even though they reside in a modules.cgz. Since I did not understand the moddep magic of the busybox kernel module tools, I did not touch the explicit loading of dependencies. (And again there would be the LCS/CTC speciality mentioned above.) > * There is code duplicated between the linuxrc and lsznet (see, eg, the > declaration of $CU) and there's already divergence between the two Wow, I'm impressed by the detail of your findings. The divergence is intentional. Lsznet was developed as a generic tool sensing all types of network adaptors independent of a use case such as in an installer environment. However, linuxrc.s390 is only interested in the supported ones, i.e. qeth, lcs (and the deprecated ctc and iucv) all with TCP/IP. With some magic we could put them in a common code base, but I refrained from doing so because it would have generated yet another (small) file both lsznet and linuxrc depend on. > * cardtype2cleartext() is the sort of thing which gets out of date and > then becomes critical must fix bugs... if this information is > important, it probably should be something we can query either with > modinfo on a module or having the kernel tell it to us directly The output of cardtype2cleartext is for information presented to the user on the screen. Nothing more, nothing less. It is not used for any program logic. I don't think we can query it automatically such as using modinfo since the sysfs values only become meaningful after establishing a ccwgroup and setting the group device online, since only then the device driver gets this information from the adaptor; in other words, the values are dynamic. Sure, the device driver could provide user-friendly strings in the sysfs attribute. However, this would require changing the existing value other software may already depend on and would break on changing the strings. That's why I'm convinced that the values won't be changed often if at all and we should be safe with a static mapping table. If a new sysfs value should appear in the future, our code will inform the user and we may easily fix it. > * Aren't we now doing udev rules for persistent device naming rather > than modprobe.conf aliases? Sorry, I don't know about that. Would that be a Rawhide/RHEL6 thing? If so, could you please point me to some more information so I can make the script more Rawhide compliant? > * Why are they disabling ipv6 autoconf? Disclaimer: IPv6 support is not (fully) functional. The part in linuxrc should be complete but support in loader is missing for transferring the configuration over to stage2. As far as I understood linuxrc, it only supports manual configuration of IPv4, i.e. no DHCP or other fancy stuff. So I coded IPv6 support the same way. Am I mistaken that we either ask the user for IP address and the like or user autoconfiguration? > * Some of the helptext (helptext_nettype() is the one I'm seeing right > now) is concerning. If something isn't supported, then we shouldn't > support it. If the code is there, it's going to be considered supported At least for RHEL5 I was told, that the code including drivers are still there since there might be customers needing those things even though they might not be officially supported by RHEL. Since we on System z are very careful not breaking any potentially old environment I made sure the code for CTC and IUCV remains there and also benefits from the improved user interface. In fact it was in the existing linuxrc and I just transferred it over. > * Also, given that these values have changed at least somewhat over > time, if there's going to be help text, there needs to be some > commitment for keeping it updated I'm all for keeping them updated. One more comment: What we now call helptext was previously part of the question presented to the user. We split that in two, so we can ask the user in a one-liner (since screen real estate is precious on a 80x25 green screen not scrolling but flipping pages) and still provide the more self explaining "helptext" on request as it was there before. While having been at it, we updated the texts already to reflect current state. > * Translators are going to hate a lot of the text. And given that this > is being shown on the line mode terminal, how many translations can > really be displayed? AFAIK, the texts in linuxrc haven't had any translations so far even though they were already previously coded with the deprecated $"..." construct of the bash. From the top of my head, I wouldn't even come up with a solution when and where to select a language before linuxrc starts running. In a nutshell, I assumed there would only be English texts and as a non-native speaker I welcome any suggestions for improvement (hoping they still fit into one line ;)). > * Is there a reason if your IPL device is FCP that you wouldn't be doing > a CD install? Shouldn't we default to detecting the CD like we do on > other arches here unless you've passed 'askmethod' You're right, there might be a case where the user IPLs the install environment from an FCP device other than a CD/DVD drive, e.g. an FCP harddisk. Such case can even be used for debugging DVD install support in linuxrc without the need for a DVD drive. However, I guess this has not been deemed a valid use case for installation which is why there is not the "usual" CD detection. I have to think about the latter, if and how something like that would be possible on s390 and especially how that would be possible in the very limited linuxrc. In any case it is necessary to set the FCP adaptor online and add port and LUN before anything can be detected by means of SCSI device type. And that is exactly what is done right now, there is nothing more in linuxrc. While writing this, I realize that the detection of a CD/DVD drive is already done platform independent by the loader! We even verified this. If you IPL from FCP disk, loader won't let you select CD install, only if you IPL from FCP CD/DVD. And I think askmethod should just work. Ain't that nice? Sometimes s390 boggles my mind. > * Checking bash versions is also silly As mentioned above, lsznet was designed independently as a generic distro-independent tool and I found many peculiarities with different bash versions. I found it more user-friendly to bail out early than to keep on running and producing errors all over since the necessary bash functionality is not there. > Okay, now that my eyes are bleeding, I'm going to be done for now. I'm sorry. Hope you cured meanwhile. :) > The > mix of new functionality, reformatting and changing the way things work > makes this essentially unreviewable for correctness so I wasn't really > able to address anything there :/ IMHO, you addressed a lot of valid points which I am thankful for. I tried to retain as much of the old code as possible but the new user interface and all the new checking which was missing before plus some rearranging for better maintenance made it virtually undiffable. :( For what it's worth, I spent a lot of effort making sure it behaves like the old linuxrc with regard to configuration outcome. Sorry for the long reply. On System z things are sometimes quite special and I felt that thorough explanations are in order. Steffen Linux on System z Development IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Erich Baier Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 _______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
