In baselayout-1.13.0_alpha7-r1's checkroot script, a faulty regular expression matching test is used to check fstab for the fsck passno for the root filesystem. The code as it exists currently: if [[ $'\n'$(</etc/fstab) =~ $'\n'.*[[:space:]]/[[:space:]].*[[:space:]].*[[:space:]](.*)[[:space:]] ]] \ && [[ ${BASH_REMATCH[1]} == "0" ]] ; then : # do the fsck else ebegin $"Skipping root filesystem check" "(fstab's passno == 0)" retval=0 fi That doesn't work here -- it executes the else clause, even tho I have a an fsck passno of 1 for / in fstab. However, my fstab is quite complex, with quite a bit of fancy-format comments, and the test as written just doesn't quite have the necessary complexity to match my fstab. Now I had to look up the bash regex match functionality in the manpage as I wasn't familiar with it, so I'm no expert, but a few tests show what's going on. All those unqualified .* wildcards appear to be matching WAAYYY more than they should be, multiple lines from the file, so the match portion saved in BASH_REMATCH[1] could end up being virtually anything and in fact ends up matching an entire line, in this case, a comment line of all hashes. One change that looks like it should be helpful would be changing all those .* matches to [^[:space:]]*, so they match everything /except/ the various space chars. I'm not sure bash's regex code supports it (I think it does but...), but changing the * to a +, "zero or more" to "one or more", would probably be closer to what's intended as well. After I attach my fstab and add the results of a few diagnostics tests to show what is actually being matched by the existing code, I'll experiment a bit and see how those suggestions go.
Created attachment 103337 [details] /etc/fstab Here's fstab. As I said, it's well annotated with comments, which of course makes parsing more difficult. A standard 8-space tab-size will aid in viewing. Also note the /. /rootbind line, allowing me to bind-mount my root partition for easier backup of the content of that partition only. Initially I thought the script was mistaking that for the standard / mount line, but it isn't.
With the attached fstab file, the following command repeatedly run with slight variations to the final BASH_REMATCH[] subscript yields the following results. This command is the script test in question, modified to yield match-captures for each field so it's possible to see what's actually matching. if [[ $'\n'$(</etc/fstab) =~ $'\n'(.*)[[:space:]](/)[[:space:]](.*)[[:space:]](.*)[[:space:]](.*)[[:space:]] ]] && [[ ${BASH_REMATCH[5]} == "0" ]] ; then :; else echo ${BASH_REMATCH[0]}; fi With the subscript on the second BASH_REMATCH set to: [0]: Unsurprisingly, the whole file is captured. [1]: The file thru the /dev/part entry for root is captured. It would appear this is as expected, so no change required, here. [2]: As expected, this is simply / on its own, as that's what it's hardcoded to. [3]: This one appears to be the trouble match. It matches nearly the entire rest of the file, thru the first ### after the /dev/fd0 line, apparently only yielding enough back to let the rest of the regex match. WAAAYYY greedier than expected! [4]: This one could be as greedy as [3], only [3] gobbled up everything already and yielded back only enough to allow a single field match here, the second ### after the /dev/fd0 line. [5]: Likewise, except that the field matched is a full line of ##### marks, one of the closing comment lines. Note that this is the only one that was originally saved and was therefore tested in the original script as [1]. The BASH_REMATCH test therefore fails since a string of hash-marks doesn't equal "0", throwing me into the else clause, which then says I have 0 set as the passno, which I don't. Time to post this and then see if I can figure out something that works.
On a side note: http://www.namesys.com/faq.html#fstab
Created attachment 103350 [details, diff] patch correcting the bad test OK, I've got that bug worked out, but ran into a couple others. The tested field was the dump field, not the fsck passno field, and after I got the right field, the test was if it was equal to "0", but zero means skip, so the if test logic is reversed in that case. Therefore, I reversed that logic. Here's a command line diagnostic that seems to work here, parallel to the one above but with the appropriate changes clearing all three bugs listed so far. It detects the correct field, fails to match if the field isn't digits-only, thereby triggering the else clause, matches but fails the BASH_REMATCH test if the field is a single "0", thereby triggering the else clause again, and matches both if the field consists only of digits other than 0, thereby triggering the main if clause. if [[ $'\n'$(</etc/fstab) =~ $'\n'.*[[:space:]]/[[:space:]]+[^[:space:]]+[[:space:]]+[^[:space:]]+[[:space:]]+[[:digit:]]+[[:space:]]+([[:digit:]]+)[[:space:]] ]] && [[ ${BASH_REMATCH[1]} != "0" ]] ; then echo if clause ${BASH_REMATCH[1]}; else echo else clause ${BASH_REMATCH[1]}; fi When I went to post that bugs wasn't responding, so I went ahead and created a patch based on it. I'm attaching it here, altho I've not tested the actual patched version yet, only the interactive thing above. I gotta do other stuff now, but will try a reboot in a few hours to test it.
Created attachment 103492 [details, diff] Replace the regex with simple test logic Lets remove the regex and use better logic. This should work with all scenarios :)
this is just more complicated for no gain the point of moving to the bash regex was to save some cycles over using awk ... if we cant sanely use the bash regex, lets revert to the awk that existed before (and worked fine)
(In reply to comment #6) > the point of moving to the bash regex was to save some cycles over using awk > ... It also allowed it to work with BSD as iirc the awk statement didn't work on BSD. I'll see if I can come up with something portable later today.
Created attachment 103538 [details, diff] Use sed How about this one then?
(In reply to comment #8, attachment id=103538, Use sed.) Tested the sed patch and it works, as my patch did. I didn't test the comment #5 version. What's the benefit of doing the sed regex against doing it with bash alone? They both look equally complex and error prone to me. The comment #5 version looks simplest to work with to me if it works (as I said, I didn't test that one), tho obviously I'm not the maintainer so that doesn't count for much. =8^) (Testing just placed my nose again in front of another bug, a local vs. UTC clock issue, so I've got another bug to file if I don't find one filed already.)
(In reply to comment #9) > What's the benefit of doing the sed regex against doing it with bash alone? > They both look equally complex and error prone to me. The bash regex does not take into account comments or a blank value for pass number as technically you don't have to supply a number for dump/pass at all. (Just realised mine doesn't either, but I'll fix that in svn) > The comment #5 version looks simplest to work with to me if it works (as I > said, I didn't test that one), tho obviously I'm not the maintainer so that > doesn't count for much. =8^) Bash loops are also slow - sadly.
(In reply to comment #10) > (In reply to comment #9) > > What's the benefit of doing the sed regex against doing it with bash alone? > > They both look equally complex and error prone to me. > > The bash regex does not take into account comments or a blank value for pass > number as technically you don't have to supply a number for dump/pass at all. > (Just realised mine doesn't either, but I'll fix that in svn) Comments are indeed a problem, yes, but I believe dump/pass is taken care of in both regex patches here (but not in the unpatched version as it currently exists) by the digit checks. If the fields aren't there, it will use the first fields from the next line. If they are comments, it won't match as the # won't match a digit, and device and mountpoint wouldn't match either, as they'd not be pure digits. The non-expanding fields three and four help with constraining as well, and an initial field check for /dev/ non-space + could be added to further constrain. The result would be a fall-thru to the else case, no-check, which according to the fstab manpage is indeed the correct no-value default. That leaves comments, however, which do seem to make the bash regex a non-starter, as there's simply no way to line-anchor and thus check for an initial #. Thus, there's no simple way (indeed no way I can think of) to avoid a match on something like the following, without killing other requirements: # /dev/hda1 / ext3 defaults 0 1 > > The comment #5 version looks simplest to work with to me if it works (as I > > said, I didn't test that one), tho obviously I'm not the maintainer so that > > doesn't count for much. =8^) > > Bash loops are also slow - sadly. Well, I'll take slow and working over fast and anyone's guess, any day. =8^) For a typical user it shouldn't be /that/ big an issue, and anyone running 256 SCSI devices or some such is at a level where a bit of customization is already likely to be necessary.
Fixed in baselayout-1.13.0_alpha8
