[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] handle upgrades on systems whose rootfs is on an encrypted logical volume (471288)



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

-- 
David Cantrell <dcantrell redhat com>
Red Hat / Honolulu, HI

Attachment: pgpEOFPlmYHMg.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]