Re: [PATCH] Integrate the python-cryptsetup package

[David Lehman <dlehman redhat com>]

On Mon, 2009-03-02 at 15:35 +0100, Martin Sivak wrote:
> ---
>  anaconda.spec                |    2 +
>  storage/devicelibs/crypto.py |  158 +++++++++--------------------------------
>  2 files changed, 37 insertions(+), 123 deletions(-)

In general, it looks good. I have a couple of comments below.

> diff --git a/storage/devicelibs/crypto.py b/storage/devicelibs/crypto.py
> index d69e7d3..e16bbe4 100644
> --- a/storage/devicelibs/crypto.py
> +++ b/storage/devicelibs/crypto.py
> @@ -17,9 +17,11 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  #
>  # Author(s): Dave Lehman <dlehman redhat com>
> +#            Martin Sivak <msivak redhat com>
>  #
>  
>  import os
> +from pycryptsetup import CryptSetup
>  
>  import iutil
>  from ..errors import *
> @@ -27,169 +29,79 @@ from ..errors import *
>  import gettext
>  _ = lambda x: gettext.ldgettext("anaconda", x)
>  
> +def askyes(question):
> +    return True
> +
> +def dolog(priority, text):
> +    pass
> +
>  def is_luks(device):
> -    rc = iutil.execWithRedirect("cryptsetup",
> -                                ["isLuks", device],
> -                                stdout = "/dev/tty5",
> -                                stderr = "/dev/tty5",
> -                                searchPath = 1)
> -    if rc:
> -        return False
> -    else:
> -        return True
> +    cs = CryptSetup(yesDialog = askyes, logFunc = dolog)
> +    return cs.isLuks(device)
>  
>  def luks_uuid(device):
> -    uuid = iutil.execWithCapture("cryptsetup",
> -                                 ["luksUUID", device],
> -                                 stderr="/dev/tty5")
> -    return uuid.strip()
> +    cs = CryptSetup(yesDialog = askyes, logFunc = dolog)
> +    return cs.luksUUID(device).strip()
>  
>  def luks_status(name):
> -    """0 means active, 1 means inactive (or non-existent)"""
> -    rc = iutil.execWithRedirect("cryptsetup",
> -                                ["status", name],
> -                                stdout = "/dev/tty5",
> -                                stderr = "/dev/tty5",
> -                                searchPath = 1)
> -    return rc
> +    """True means active, False means inactive (or non-existent)"""
> +    cs = CryptSetup(yesDialog = askyes, logFunc = dolog)
> +    return (cs.luksStatus(device)!=None)
>  
>  def luks_format(device,
>                  passphrase=None, key_file=None,
>                  cipher=None, key_size=None):
> -    p = os.pipe()
> -    argv = ["-q"]
> -    os.close(p[1])
> -
> -    if cipher:
> -        argv.extend(["--cipher", cipher])
> +    cs = CryptSetup(yesDialog = askyes, logFunc = dolog)
> +    key_file_unlink = False
>  
> -    if key_size:
> -        argv.append("--key-size=%d" % key_size)
> -
> -    argv.extend(["luksFormat", device])
> -        
>      if passphrase:
> -        os.write(p[1], "%s\n" % passphrase)
> +        key_file = cs.prepare_passphrase_file(passphrase)
> +        key_file_unlink = True
>      elif key_file and os.path.isfile(key_file):
>          argv.append(key_file)
>      else:
>          raise ValueError("luks_format requires either a passphrase or a key file")
>  
> -    rc = iutil.execWithRedirect("cryptsetup",
> -                                argv,
> -                                stdin = p[0],
> -                                stdout = "/dev/tty5",
> -                                stderr = "/dev/tty5",
> -                                searchPath = 1)
> +    rc = cs.luksFormat(device = device, cipher = cipher, keysize = key_size, keyfile = key_file)

A line break at or before column 79 would make this easier to look at in
an editor, but it isn't required.

> +    if key_file_unlink: os.unlink(key_file)

I'm not crazy about this style-wise, but it isn't wrong.

Dave