On Wed, Nov 12, 2008 at 05:45:40PM -0600, Dave Lehman wrote:
> Another upgrade case I didn't catch earlier: root on encrypted LV. Since
> it's an LV, we use the device in fstab, not a UUID. So, fsset.readFstab
> doesn't know how to get from /dev/mapper/luks-$lvname
> to /dev/mapper/$lvname. This patch adds a new function to retrieve a
> backing device from /etc/crypttab so we can create a usable Device
> instance and thus successfully upgrade.
This patch looks ok to me, but I have comments below.
>
> diff --git a/fsset.py b/fsset.py
> index 40eb0d2..31c623e 100644
> --- a/fsset.py
> +++ b/fsset.py
> @@ -2695,6 +2695,21 @@ def makeDevice(dev):
> device = PartitionDevice(dev, encryption=cryptoDev)
> return device
>
> +def findBackingDevInCrypttab(mappingName):
> + backingDev = None
> + try:
> + lines = open("/mnt/sysimage/etc/crypttab").readlines()
> + except IOError, e:
> + pass
> + else:
> + for line in lines:
> + fields = line.split()
> + if fields[0] == mappingName:
> + backingDev = fields[1]
> + break
> +
> + return backingDev
> +
The readlines() and split() still preserves the newline character in crypttab,
which is only a problem if fields[1] is the last field. Something to
consider. I'd do 'line = line.strip()' before the split() call.
Also, 'backingDev = fields[1]' could throw an exception if fields only
contains one member. Either a length check+continue or something similar
could avoid that.
> # XXX fix RAID
> def readFstab (anaconda):
> def createMapping(dict):
> @@ -2833,6 +2848,39 @@ def readFstab (anaconda):
> if loopIndex.has_key(device):
> (dev, fs) = loopIndex[device]
> device = LoopbackDevice(dev, fs)
> + elif fields[0].startswith("/dev/mapper/luks-"):
> + backingDev = findBackingDevInCrypttab(fields[0][12:])
> + log.debug("device %s has backing device %s" % (fields[0],
> + backingDev))
> + if backingDev is None:
> + log.error("unable to resolve backing device for %s" %
> fields[0]
> + continue
> + elif backingDev.startswith('LABEL='):
> + label = backingDev[6:]
> + if label in labelDupes:
> + showError(label, intf)
> +
> + if labelToDevice.has_key(label):
> + device = makeDevice(labelToDevice[label])
While I'm ok with this code, lines like this:
backingDev = findBackingDevInCrypttab(fields[0][12:])
label = backingDev[6:]
Are more difficult to figure out later on. Why are we reading from that
position to the end? What does sample input look like?
I can't complain too much, I mean, I've written code like this many times
before. Only throwing it out there as a construct we should avoid in the
future.
> def createMapping(dict):
> @@ -2833,6 +2848,39 @@ def readFstab (anaconda):
> if loopIndex.has_key(device):
> (dev, fs) = loopIndex[device]
> device = LoopbackDevice(dev, fs)
> + elif fields[0].startswith("/dev/mapper/luks-"):
> + backingDev = findBackingDevInCrypttab(fields[0][12:])
> + log.debug("device %s has backing device %s" % (fields[0],
> + backingDev))
> + if backingDev is None:
> + log.error("unable to resolve backing device for %s" %
> fields[0]
> + continue
> + elif backingDev.startswith('LABEL='):
> + label = backingDev[6:]
> + if label in labelDupes:
> + showError(label, intf)
> +
> + if labelToDevice.has_key(label):
> + device = makeDevice(labelToDevice[label])
> + else:
> + log.warning ("crypttab file has LABEL=%s, but this
> label "
> + "could not be found on any file
> system", label
> + # bad luck, skip this entry.
> + continue
> + elif backingDev.startswith('UUID='):
> + uuid = backingDev[5:]
> + if uuid in uuidDupes:
> + showError(uuid, intf)
> +
> + if uuidToDevice.has_key(uuid):
> + device = makeDevice(uuidToDevice[uuid])
> + else:
> + log.warning ("crypttab file has UUID=%s, but this
> UUID"
> + "could not be found on any file
> system", uuid)
> + # bad luck, skip this entry.
> + continue
> + else:
> + device = makeDevice(backingDev[5:])
Same comment as above for the similar lines in this block.
> elif fields[0].startswith('/dev/'):
> # Older installs may have lines starting with things
> like /dev/proc
> # so watch out for that on upgrade.
>
>
> Dave
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI
Attachment:
pgpzDnJ1wfPLQ.pgp
Description: PGP signature
_______________________________________________ Anaconda-devel-list mailing list Anaconda-devel-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/anaconda-devel-list
