curtin-dev team mailing list archive

[Dan Watkins <daniel.watkins@xxxxxxxxxxxxx>]

Review: Needs Fixing

Broadly speaking this looks good, thanks for your work!  I do have a few comments inline (though none of them are enormous).

Diff comments:

> diff --git a/curtin/udev.py b/curtin/udev.py
> index e2e3dd0..0908bc9 100644
> --- a/curtin/udev.py
> +++ b/curtin/udev.py
> @@ -4,7 +4,14 @@ import shlex
>  import os
>  
>  from curtin import util
> -from curtin.log import logged_call
> +from curtin.log import logged_call, LOG
> +
> +try:
> +    shlex_quote = shlex.quote
> +except AttributeError:
> +    # python2.7 shlex does not have quote, give it a try
> +    def shlex_quote(value):

Looking at six's documentation (search for shlex.quote in https://six.readthedocs.io/) and then Python's own (https://docs.python.org/2/library/pipes.html#pipes.quote), shlex.quote is actually a move of pipes.quote from 2.7, so could we use that instead of defining our own?

(I had never heard of the pipes module until right now, in case you were wondering. :p)

> +        return ("'" + value.replace("'", "\'\"\'\"\'") + "'")
>  
>  
>  def compose_udev_equality(key, value):
> @@ -90,7 +97,22 @@ def udevadm_info(path=None):
>              value = None
>          if value:
>              # preserve spaces in values to match udev database
> -            parsed = shlex.split(value)
> +            try:
> +                parsed = shlex.split(value)
> +            except ValueError:
> +                try:
> +                    # strip the leading/ending single tick from udev output
> +                    quoted = shlex_quote(value[1:-1])
> +                    LOG.debug('udevadm_info: quoting shell-escape chars '
> +                              'in %s=%s -> %s', key, value, quoted)
> +                    parsed = shlex.split(quoted)
> +                except ValueError:
> +                    # strip the leading/ending single tick from udev output

This is the same comment as above, but for a different line; can we make it distinguish the behaviour here from the behaviour above?

(And a separate nit, should we just do `value = value[1:-1]` at the top of the outermost exception block so we aren't repeating ourselves?)

> +                    escaped_value = (
> +                        value[1:-1].replace("'", "_").replace('"', "_"))
> +                    LOG.debug('udevadm_info: replacing shell-escape chars '
> +                              'in %s=%s -> %s', key, value, escaped_value)
> +                    parsed = shlex.split(escaped_value)
>              if ' ' not in value:
>                  info[key] = parsed[0]
>              else:
> diff --git a/tests/unittests/test_udev.py b/tests/unittests/test_udev.py
> index 33d5f44..0c6c5b9 100644
> --- a/tests/unittests/test_udev.py
> +++ b/tests/unittests/test_udev.py
> @@ -54,6 +55,19 @@ class TestUdevInfo(CiTestCase):
>              capture=True)
>          self.assertEqual(sorted(INFO_DICT), sorted(info))
>  
> +    @mock.patch('curtin.util.subp')
> +    def test_udevadm_info_escape_quotes(self, m_subp):
> +        """verify we escape quotes when we fail to split. """
> +        mypath = '/dev/sdz'
> +        datafile = 'tests/data/udevadm_info_sandisk_cruzer.txt'
> +        m_subp.return_value = (util.load_file(datafile), "")
> +        info = udev.udevadm_info(mypath)
> +        m_subp.assert_called_with(
> +            ['udevadm', 'info', '--query=property', '--export', mypath],
> +            capture=True)
> +        self.assertEqual('SanDisk'"'"'', info['SCSI_VENDOR'])

`'SanDisk'"'"''` is a long-winded way of expressing "Sandisk'" (or 'Sandisk\'' if you're very committed to single quotes):

In [1]: 'SanDisk'"'"''                                                                                                                                                                                                                                                                    
Out[1]: "SanDisk'"

(I think "Sandisk'" is the value that we should check against, so this isn't a correctness issue, but I would expect future readers to stumble in the same way.)

> +        self.assertEqual('SanDisk'"'"'', info['SCSI_VENDOR_ENC'])
> +
>      def test_udevadm_info_no_path(self):
>          """ udevadm_info raises ValueError for invalid path value"""
>          mypath = None


-- 
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/382993
Your team curtin developers is subscribed to branch curtin:master.